-
Notifications
You must be signed in to change notification settings - Fork 8.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support adding prefix to Dubbo service resource name in Sentinel Dubbo Adapter #859
Conversation
using the LongAdder rather than AtomicInteger to Provides better performance
merge master
merge online
merge onlie
Codecov Report
@@ Coverage Diff @@
## master #859 +/- ##
============================================
+ Coverage 42.06% 42.81% +0.75%
- Complexity 1410 1461 +51
============================================
Files 307 312 +5
Lines 8868 9017 +149
Branches 1200 1226 +26
============================================
+ Hits 3730 3861 +131
- Misses 4681 4691 +10
- Partials 457 465 +8
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to add the feature in both sentinel-dubbo-adapter
(for 2.5.x-2.6.x) and sentinel-apache-dubbo-adapter
(for 2.7.x+).
*/ | ||
public final class DubboConfig { | ||
|
||
public static final String DUBBO_USE_PREFIX = "csp.sentinel.dubbo.use.prefix"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe csp.sentinel.dubbo.use.prefix
-> csp.sentinel.dubbo.resource.use.prefix
is clearer? (though too long...)
So as the following two keys.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
config keys had modified
|
||
public static String getDubboProviderPrefix() { | ||
if (usePrefix) { | ||
return SentinelConfig.getConfig(DUBBO_PROVIDER_PREFIX); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we provide a default pattern (e.g. dubbo:provider/consumer:
+ the origin name) if the usePrefix
is enabled but no prefix is provided?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we provide a default pattern (e.g.
dubbo:provider/consumer:
+ the origin name) if theusePrefix
is enabled but no prefix is provided?
default prefix had provided
Could you please also add these implementations in |
implementations had supplied |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Thanks for contributing! |
* [RIP-9] Add Operations_Trace.md in recketmq
Does this pull request fix one issue?
Fix #427
Describe how you did it
add three configs:
csp.sentinel.dubbo.use.prefix
use to control the prefix if take effect or notcsp.sentinel.dubbo.provider.prefix
config the dubbo provider service resource prefixcsp.sentinel.dubbo.consumer.prefix
config the dubbo consumer service resource prefixDescribe how to verify it
i offer a test :
com.alibaba.csp.sentinel.adapter.dubbo.DubboUtilsTest#testGetResourceNameWithPrefix
Special notes for reviews
none