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 data source integrate with spring cloud config #899

Merged
merged 43 commits into from
Aug 20, 2019
Merged

Support data source integrate with spring cloud config #899

merged 43 commits into from
Aug 20, 2019

Conversation

linlinisme
Copy link
Collaborator

@linlinisme linlinisme commented Jul 9, 2019

###Describe what this PR does / why we need it

Now, data source support nacos,zeepkeeper,redis and so on.The spring-cloud-config also should be.

Does this pull request fix one issue?

Fix issue: #831

Describe how you did it

My proposal is based on spring cloud config original loading mechanism.
The PropertySourceLocator Interface : provides external attribute loading extension points.

Any class implements PropertySourceLocator interface and modified with corresponding annotation.
It will be loaded by org.springframework.cloud.bootstrap.config.PropertySourceBootstrapConfiguration and invoke the org.springframework.cloud.bootstrap.config.PropertySourceLocator#locate to pull configurations from remote server. The spring-cloud-config example is org.springframework.cloud.config.client.ConfigServicePropertySourceLocator

All I do it is reference ConfigServicePropertySourceLocator build another client which can integrate with sentinel data source and support dynamic refresh.

It's name is com.alibaba.csp.sentinel.datasource.spring.cloud.config.SentinelRuleLocator. It will be created when spring application start and pull configurations. It supply refresh method to support dynamic refresh function.

Describe how to verify it

In sentinel-extension/sentinel-datasource-spring-cloud-config/src/test package, there is a ConfigServer and ConfigClient. You should start the ConfigServer first. It will connected my git warehouse. If you want to use another, modify config-server-application.properties file. Then you can run SentinelRuleLocatorTests. The ConfigClient also can test function. When you start ConfigClient , remember that rename client-bootstrap.yml to bootstrap.yml.Then you can through SpringCouldDataSourceTestController to test SpringCloundConfigDataSource.
The page access url is : localhost:8080/index

Special notes for reviews

If this proposal is ok, more test cases and readme file will be provided later

Lin.Liang and others added 30 commits May 9, 2019 14:54
using the LongAdder rather than AtomicInteger to Provides better performance
@codecov-io
Copy link

codecov-io commented Jul 9, 2019

Codecov Report

Merging #899 into master will decrease coverage by 0.26%.
The diff coverage is 28.4%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #899      +/-   ##
============================================
- Coverage     42.74%   42.48%   -0.27%     
- Complexity     1450     1458       +8     
============================================
  Files           311      315       +4     
  Lines          9047     9223     +176     
  Branches       1228     1258      +30     
============================================
+ Hits           3867     3918      +51     
- Misses         4706     4814     +108     
- Partials        474      491      +17
Impacted Files Coverage Δ Complexity Δ
...ring/cloud/config/SpringCloudConfigDataSource.java 0% <0%> (ø) 0 <0> (?)
...ource/spring/cloud/config/SentinelRuleStorage.java 0% <0%> (ø) 0 <0> (?)
...onfig/config/DataSourceBootstrapConfiguration.java 100% <100%> (ø) 2 <2> (?)
...ource/spring/cloud/config/SentinelRuleLocator.java 35.87% <35.87%> (ø) 5 <5> (?)
...ava/com/alibaba/csp/sentinel/node/ClusterNode.java 100% <0%> (+4.76%) 8% <0%> (+1%) ⬆️

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 ec30e89...17b898e. Read the comment docs.

@sczyh30 sczyh30 added area/integrations Issues or PRs related to integrations with open-source components to-review To review labels Jul 9, 2019
@sczyh30
Copy link
Member

sczyh30 commented Jul 26, 2019

Would you like to provide some suggestions on this? @fangjian0423

@sczyh30 sczyh30 added the integration/spring-cloud Issue related to Spring Cloud Alibaba integration label Jul 26, 2019
@linlinisme
Copy link
Collaborator Author

hi, is there some progress?
 

@sczyh30
Copy link
Member

sczyh30 commented Jul 30, 2019

hi, is there some progress?

I've reviewed the code last week, and I'll need to test the sample these days :)

@sczyh30
Copy link
Member

sczyh30 commented Aug 3, 2019

Nice work. I'm wondering how this could achieve dynamic updating? It seems when the content of PropertySource is updated, the property inside the SpringCloudConfigDataSource is not notified (via updateValue)?

@linlinisme
Copy link
Collaborator Author

linlinisme commented Aug 3, 2019

Nice work. I'm wondering how this could achieve dynamic updating? It seems when the content of PropertySource is updated, the property inside the SpringCloudConfigDataSource is not notified (via updateValue)?

They dynamic updating function is based on the Web Hook call back. When you change config in the Server does you setting Web Hook call back url ? I offer a test in com.alibaba.csp.sentinel.datasource.spring.cloud.config.test.SpringCouldDataSourceTest#refresh 。Actually the dynamic updating function need users develop a call back function, but it is simple :), and we can offer the sample to them.

@linlinisme
Copy link
Collaborator Author

Nice work. I'm wondering how this could achieve dynamic updating? It seems when the content of PropertySource is updated, the property inside the SpringCloudConfigDataSource is not notified (via updateValue)?

listener had added

@sczyh30
Copy link
Member

sczyh30 commented Aug 14, 2019

How could the client perceive that the remote config has changed? By binding a git webhook with the refresh API?

@linlinisme
Copy link
Collaborator Author

How could the client perceive that the remote config has changed? By binding a git webhook with the refresh API?

Yes, it is.

@linlinisme
Copy link
Collaborator Author

some process?

@sczyh30
Copy link
Member

sczyh30 commented Aug 18, 2019

How could the client perceive that the remote config has changed? By binding a git webhook with the refresh API?

Yes, it is.

Could you please add a brief instruction for this in the document?

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 merged commit 30158bc into alibaba:master Aug 20, 2019
@sczyh30 sczyh30 removed the to-review To review label Aug 20, 2019
@sczyh30 sczyh30 added this to the 1.7.0 milestone Aug 20, 2019
@sczyh30
Copy link
Member

sczyh30 commented Aug 20, 2019

Nice work. Thanks for contributing!

CST11021 pushed a commit to CST11021/Sentinel that referenced this pull request Nov 3, 2021
[RIP-10] Add test case for QueryCorrectionOffsetBody & ResetOffsetBody
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 integration/spring-cloud Issue related to Spring Cloud Alibaba integration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants