Skip to content
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

Merged
merged 10 commits into from Aug 14, 2019
Merged

Support adding prefix to Dubbo service resource name in Sentinel Dubbo Adapter #859

merged 10 commits into from Aug 14, 2019

Conversation

linlinisme
Copy link
Collaborator

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 not
csp.sentinel.dubbo.provider.prefix config the dubbo provider service resource prefix
csp.sentinel.dubbo.consumer.prefix config the dubbo consumer service resource prefix

Describe how to verify it

i offer a test : com.alibaba.csp.sentinel.adapter.dubbo.DubboUtilsTest#testGetResourceNameWithPrefix

Special notes for reviews

none

@codecov-io
Copy link

codecov-io commented Jun 24, 2019

Codecov Report

Merging #859 into master will increase coverage by 0.75%.
The diff coverage is 95.23%.

Impacted file tree graph

@@             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
Impacted Files Coverage Δ Complexity Δ
...nel/adapter/dubbo/SentinelDubboProviderFilter.java 64% <100%> (ø) 2 <0> (ø) ⬇️
...nel/adapter/dubbo/SentinelDubboConsumerFilter.java 50% <100%> (ø) 2 <0> (ø) ⬇️
...alibaba/csp/sentinel/adapter/dubbo/DubboUtils.java 95.45% <100%> (+1.7%) 7 <2> (+2) ⬆️
...nel/adapter/dubbo/SentinelDubboConsumerFilter.java 57.14% <100%> (ø) 2 <0> (ø) ⬇️
...nel/adapter/dubbo/SentinelDubboProviderFilter.java 57.14% <100%> (ø) 2 <0> (ø) ⬇️
...sp/sentinel/adapter/dubbo/AbstractDubboFilter.java 100% <100%> (ø) 6 <2> (+2) ⬆️
...csp/sentinel/adapter/dubbo/config/DubboConfig.java 90% <90%> (ø) 7 <7> (?)
...csp/sentinel/adapter/dubbo/config/DubboConfig.java 90% <90%> (ø) 7 <7> (?)
...otation/aspectj/AbstractSentinelAspectSupport.java 66.91% <0%> (-2.26%) 37% <0%> (-1%)
...ain/java/com/alibaba/csp/sentinel/log/LogBase.java 56.94% <0%> (-1.63%) 10% <0%> (-1%)
... and 18 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dca4440...b08859a. Read the comment docs.

@sczyh30 sczyh30 added the to-review To review label Jun 24, 2019
Copy link
Member

@sczyh30 sczyh30 left a 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";
Copy link
Member

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.

Copy link
Collaborator Author

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);
Copy link
Member

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?

Copy link
Collaborator Author

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?

default prefix had provided

@sczyh30 sczyh30 added the area/integrations Issues or PRs related to integrations with open-source components label Aug 3, 2019
@sczyh30
Copy link
Member

sczyh30 commented Aug 5, 2019

Could you please also add these implementations in sentinel-dubbo-adapter (adapter of Dubbo 2.5.x-2.6.x)?

@linlinisme
Copy link
Collaborator Author

Could you please also add these implementations in sentinel-dubbo-adapter (adapter of Dubbo 2.5.x-2.6.x)?

implementations had supplied

Copy link
Member

@sczyh30 sczyh30 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@sczyh30 sczyh30 removed the to-review To review label Aug 14, 2019
@sczyh30 sczyh30 merged commit 39293c1 into alibaba:master Aug 14, 2019
@sczyh30
Copy link
Member

sczyh30 commented Aug 14, 2019

Thanks for contributing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/integrations Issues or PRs related to integrations with open-source components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support adding prefix to Dubbo service resource name in Sentinel Dubbo Adapter
3 participants