fix: For idl stub service, use idl service name for discovery (#15365)
What is the purpose of the change?
fix: For idl stub service, use idl service name for discovery (#15365)
Checklist
- [x] Make sure there is a GitHub_issue field for the change.
- [x] Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
- [x] 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.
- [x] Make sure gitHub actions can pass. Why the workflow is failing and how to fix it?
Codecov Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 60.90%. Comparing base (2af3a91) to head (fee6c4f).
:warning: 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%) |
:arrow_down: |
Flags with carried forward coverage won't be shown. Click here to find out more.
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
:rocket: New features to boost your workflow:
- :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
- :package: JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
could you submit a new pr at https://github.com/apache/dubbo-samples to add a new sample module to confirm this pr?
could you submit a new pr at https://github.com/apache/dubbo-samples to add a new sample module to confirm this pr?
https://github.com/apache/dubbo-samples/pull/1220
could you submit a new pr at https://github.com/apache/dubbo-samples to add a new sample module to confirm this pr?
i will test it.
@heliang666s @oxsean PTAL i tested https://github.com/apache/dubbo-samples/pull/1220 with this pr and got successful result.
version compatibility should also be considered.
| client | server |
|---|---|
| old | new |
| new | old |
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?
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.
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...
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.