dubbo icon indicating copy to clipboard operation
dubbo copied to clipboard

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

Open lqscript opened this issue 6 months ago • 10 comments

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?

lqscript avatar Jun 11 '25 11:06 lqscript

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.

codecov-commenter avatar Jun 11 '25 11:06 codecov-commenter

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

zrlw avatar Jun 12 '25 06:06 zrlw

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

lqscript avatar Jun 12 '25 07:06 lqscript

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 avatar Jun 12 '25 14:06 zrlw

@heliang666s @oxsean PTAL i tested https://github.com/apache/dubbo-samples/pull/1220 with this pr and got successful result.

zrlw avatar Jun 13 '25 04:06 zrlw

version compatibility should also be considered.

client server
old new
new old

zrlw avatar Jun 14 '25 05:06 zrlw

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?

lqscript avatar Jun 16 '25 06:06 lqscript

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.

zrlw avatar Jun 17 '25 01:06 zrlw

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...

lqscript avatar Jun 18 '25 05:06 lqscript

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.

lqscript avatar Jun 26 '25 10:06 lqscript