Skip to content

Conversation

@lqscript
Copy link

What is the purpose of the change?

fix: For idl stub service, use idl service name for discovery (#15365)

Checklist

  • Make sure there is a GitHub_issue field for the change.
  • Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
  • Write necessary unit-test to verify your logic correction. If the new feature or significant change is committed, please remember to add sample in dubbo samples project.
  • Make sure gitHub actions can pass. Why the workflow is failing and how to fix it?

@codecov-commenter
Copy link

codecov-commenter commented Jun 11, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 60.90%. Comparing base (2af3a91) to head (fee6c4f).
⚠️ Report is 90 commits behind head on 3.3.

Additional details and impacted files
@@             Coverage Diff              @@
##                3.3   #15449      +/-   ##
============================================
+ Coverage     60.74%   60.90%   +0.15%     
- Complexity    10934    11444     +510     
============================================
  Files          1887     1888       +1     
  Lines         86242    86332      +90     
  Branches      12927    12946      +19     
============================================
+ Hits          52391    52582     +191     
+ Misses        28377    28313      -64     
+ Partials       5474     5437      -37     
Flag Coverage Δ
integration-tests ?
integration-tests-java17 33.10% <85.71%> (?)
integration-tests-java8 33.14% <85.71%> (?)
samples-tests ?
samples-tests-java17 31.47% <42.85%> (?)
samples-tests-java8 29.37% <42.85%> (?)
unit-tests 58.83% <85.71%> (-0.06%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@zrlw
Copy link
Contributor

zrlw commented Jun 12, 2025

could you submit a new pr at https://github.com/apache/dubbo-samples to add a new sample module to confirm this pr?

@lqscript
Copy link
Author

could you submit a new pr at https://github.com/apache/dubbo-samples to add a new sample module to confirm this pr?

apache/dubbo-samples#1220

@zrlw
Copy link
Contributor

zrlw commented Jun 12, 2025

could you submit a new pr at https://github.com/apache/dubbo-samples to add a new sample module to confirm this pr?

apache/dubbo-samples#1220

i will test it.

@zrlw
Copy link
Contributor

zrlw commented Jun 13, 2025

@heliang666s @oxsean PTAL
i tested apache/dubbo-samples#1220 with this pr and got successful result.

@zrlw
Copy link
Contributor

zrlw commented Jun 14, 2025

version compatibility should also be considered.

client server
old new
new old

@lqscript
Copy link
Author

version compatibility should also be considered.

client server
old new
new old

Given issue #15365, this incompatibility should only occur in interface-level service discovery scenarios where package and java_package differ. And when users want to migrate to application-level service discovery, they will also encounter this compatibility issue.

Based on the official documentation (official documentation), it clearly states that package should be used for service discovery, which is why I believe this should be treated as a bug fix.

So maybe I should only change this in application-level service discovery, but leave the interface-level service discovery as it is to maintain compatibility with existing clients.

Since I'm relatively new to Dubbo, what would you recommend as the most appropriate way to handle this compatibility issue while still fixing the underlying problem?

@zrlw
Copy link
Contributor

zrlw commented Jun 17, 2025

In my opinion, It is best to provide users with an option, just like dubbo.registry.register-mode, users could choose instance(only new), interface(only old) or all(export both instance and interface for compatibility) according to their own situation.

@lqscript
Copy link
Author

I looked at the source code, and if i make this change, it would require modifications in many places...
I think if incompatibility issues are actually encountered, changing the package name would be a simpler workaround...

@zrlw zrlw added help wanted Everything needs help from contributors type/proposal Everything you want Dubbo have labels Jun 24, 2025
@lqscript
Copy link
Author

I found that the serviceKey is not only used for service discovery, but also for routing and circuit breaking. Therefore, using the all approach may not be appropriate, as some user configurations would also need to be updated accordingly.

How about adding a boolean value to indicate whether to use the IDL package name as the serviceKey? This way, existing services do not need to be changed, and new services can use the IDL package name as the serviceKey.

@zrlw zrlw closed this Jul 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

help wanted Everything needs help from contributors type/proposal Everything you want Dubbo have

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants