feat: Connect spring cloud properties refresh #4303

gy09535 · 2022-04-07T03:58:57Z

What's the purpose of this PR

通过整合 Spring Cloud 来完成对如下内容更新：

完成对于 @RefreshScope 的自动更新。
完成对于 @ConfigurationProperties 配置类的自动更新。
完成对于日志的重新绑定。

参考：

Which issue(s) this PR fixes:

Fixes #4293

Follow this checklist to help us incorporate your contribution quickly and easily:

Read the Contributing Guide before making this pull request.
Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
Write necessary unit tests to verify the code.
Run mvn clean test to make sure this pull request doesn't break anything.
Update the CHANGES log.

codecov-commenter · 2022-04-07T04:03:25Z

Codecov Report

Merging #4303 (0d5a357) into master (9b1cd8f) will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master    #4303   +/-   ##
=========================================
  Coverage     53.37%   53.37%           
+ Complexity     2684     2681    -3     
=========================================
  Files           490      490           
  Lines         15252    15252           
  Branches       1586     1586           
=========================================
  Hits           8141     8141           
- Misses         6552     6555    +3     
+ Partials        559      556    -3

Impacted Files	Coverage Δ
...framework/apollo/openapi/entity/ConsumerAudit.java	`42.42% <0.00%> (-6.07%)`	⬇️
...o/openapi/filter/ConsumerAuthenticationFilter.java	`94.11% <0.00%> (-5.89%)`	⬇️
...mework/apollo/openapi/service/ConsumerService.java	`53.38% <0.00%> (-1.70%)`	⬇️
...ework/apollo/internals/RemoteConfigRepository.java	`88.34% <0.00%> (+3.06%)`	⬆️

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 9b1cd8f...0d5a357. Read the comment docs.

nobodyiam · 2022-04-08T01:02:31Z

...va/com/ctrip/framework/apollo/spring/property/SpringCloudAutoUpdateConfigChangeListener.java

+      ConfigurableListableBeanFactory beanFactory,
+      ApplicationContext applicationContext) {
+    super(environment, beanFactory);
+    this.applicationContext = applicationContext;


Is it possible to retrieve the RefreshScope bean here and cache it?

Is it possible to retrieve the RefreshScope bean here and cache it?

fixed

nobodyiam · 2022-04-08T01:51:25Z

...va/com/ctrip/framework/apollo/spring/property/SpringCloudAutoUpdateConfigChangeListener.java

+  private void adapterSpringCloud(ConfigChangeEvent changeEvent) {
+    RefreshScope refreshScope = applicationContext.getBean(RefreshScope.class);
+    if (refreshScope != null) {
+      refreshScope.refreshAll();


refreshAll is kind of a dangerous operation, it could cause unexpected behaviors, e.g. eureka service up and down. So I think we need to allow users to customize the change listener behaviors.

There was a config apollo.autoUpdateInjectedSpringProperties to control whether AutoUpdateConfigChangeListener is enabled or not. I think we need to also have two configs to control whether refreshScope.refreshAll and applicationContext.publishEvent are enabled respectively.

With that said, I would suggest we refactor the change listener logic instead of adding a new SpringCloudConfigPropertySourceProcessor

Define a Service Provider Interface, e.g. SpringAutoUpdateConfigChangeListener

Implement different implementations: AutoUpdateConfigChangeListener(the current one), SpringCloudRefreshScopeConfigChangeListener, SpringCloudEnviromentChangeEventConfigChangeListener

Use ServiceBootstrap#loadAllOrdered method to load all the implementations in PropertySourcesProcessor#initializeAutoUpdatePropertiesFeature

Write a CompositeConfigChagneListener to trigger each of the change listener

In the onChange method, each listener will first check whether itself is enabled, then will do the actual work if enabled.

The benefit of this architecture is it's easy to add more customized config change listeners in the future.

If you like this idea, I think I could contribute to the refactoring part.

Yeah, Sounds good, l prefer it. Maybe we can base this code and create another PR to do this.

I submitted #4305 to enable the spi mechanism, please help to take a look.

I submitted #4305 to enable the spi mechanism, please help to take a look.

Got it.

nobodyiam · 2022-04-08T01:54:07Z

apollo-client/pom.xml

+		<dependency>
+			<groupId>org.springframework.cloud</groupId>
+			<artifactId>spring-cloud-context</artifactId>
+			<scope>provided</scope>
+		</dependency>


I think optional is more appropriate and it's better to move it up with the spring-boot dependencies.

Suggested change

<dependency>

<groupId>org.springframework.cloud</groupId>

<artifactId>spring-cloud-context</artifactId>

<scope>provided</scope>

</dependency>

<dependency>

<groupId>org.springframework.cloud</groupId>

<artifactId>spring-cloud-context</artifactId>

<optional>true</optional>

</dependency>

I think optional is more appropriate and it's better to move it up with the spring-boot dependencies.

fixed

Anilople · 2022-04-08T11:15:09Z

Do we need to create a new maven module like spring-cloud-starter-apollo-client to implement this feature?

i.e let user choose client base on their runtime.

And the code will be more clear? i.e we know this feature only enable in spring cloud

nobodyiam · 2022-04-08T13:05:27Z

Do we need to create a new maven module like spring-cloud-starter-apollo-client to implement this feature?

i.e let user choose client base on their runtime.

And the code will be more clear? i.e we know this feature only enable in spring cloud

@Anilople This is a good idea. I will do the refactoring mentioned in this thread in apollo-client module and I think the spring-cloud-starter-apollo-client module could then enable more spring cloud features based on the spi mechanism?

nobodyiam · 2022-04-08T00:56:03Z

...-client/src/main/java/com/ctrip/framework/apollo/spring/config/PropertySourcesProcessor.java

    List<ConfigPropertySource> configPropertySources = configPropertySourceFactory.getAllConfigPropertySources();
    for (ConfigPropertySource configPropertySource : configPropertySources) {
-      configPropertySource.addChangeListener(autoUpdateConfigChangeListener);
+      configPropertySource.addChangeListener(configChangeListener(this.environment, beanFactory));


why do we create a new config change listener for every configPropertySource?

why do we create a new config change listener for every configPropertySource?

fixed

nobodyiam · 2022-04-09T12:04:37Z

...va/com/ctrip/framework/apollo/spring/property/SpringCloudAutoUpdateConfigChangeListener.java

+  private void adapterSpringCloud(ConfigChangeEvent changeEvent) {
+    RefreshScope refreshScope = applicationContext.getBean(RefreshScope.class);
+    if (refreshScope != null) {
+      refreshScope.refreshAll();


I submitted #4305 to enable the spi mechanism, please help to take a look.

Anilople · 2022-04-09T15:07:18Z

Do we need to create a new maven module like spring-cloud-starter-apollo-client to implement this feature?
i.e let user choose client base on their runtime.
And the code will be more clear? i.e we know this feature only enable in spring cloud

@Anilople This is a good idea. I will do the refactoring mentioned in this thread in apollo-client module and I think the spring-cloud-starter-apollo-client module could then enable more spring cloud features based on the spi mechanism?

Spi sounds a good idea.

….com/gy09535/apollo into feature/support-spring-cloud-refresh

stale · 2022-05-15T04:54:57Z

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 14 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

nobodyiam · 2022-05-15T06:18:47Z

@gy09535 as apollo 2.0.0 is released, we may continue this pr to create a separate spring cloud client repository?

gy09535 · 2022-06-14T08:06:08Z

@gy09535 as apollo 2.0.0 is released, we may continue this pr to create a separate spring cloud client repository?
yeah, I will do it.

nobodyiam · 2022-06-15T23:48:20Z

@gy09535 @Anilople

I'm thinking we could create a new repo under apolloconfig to host spring-cloud-apollo, how do you think?

connect spring cloud refresh

d10f29b

remove useless code

bdd3a6e

nobodyiam requested changes Apr 8, 2022

View reviewed changes

gy09535 requested a review from nobodyiam April 8, 2022 02:14

gy09535 and others added 2 commits April 8, 2022 10:39

fix application listener

2ecea07

Merge branch 'master' into feature/support-spring-cloud-refresh

c4c7355

nobodyiam reviewed Apr 9, 2022

View reviewed changes

gy09535 added 2 commits April 11, 2022 11:27

fix listener create issue

e57c5f1

Merge branch 'feature/support-spring-cloud-refresh' of https://github…

127b41a

….com/gy09535/apollo into feature/support-spring-cloud-refresh

stale bot added the stale label May 15, 2022

stale bot removed the stale label May 15, 2022

gy09535 changed the title ~~feat: Connect spring cloud properties refresh~~ WIP:feat: Connect spring cloud properties refresh Jun 14, 2022

gy09535 changed the title ~~WIP:feat: Connect spring cloud properties refresh~~ WIP(feat): Connect spring cloud properties refresh Jun 14, 2022

gy09535 added 4 commits June 15, 2022 18:44

Merge branch 'master' into feature/support-spring-cloud-refresh

ef94892

refactor apollo client

c9f9f3c

add copyright

35092b7

add new line

0d5a357

add cloud listener

08cda42

gy09535 changed the title ~~WIP(feat): Connect spring cloud properties refresh~~ feat: Connect spring cloud properties refresh Jun 16, 2022

gy09535 closed this Jun 16, 2022

github-actions bot locked and limited conversation to collaborators Jun 16, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Connect spring cloud properties refresh #4303

feat: Connect spring cloud properties refresh #4303

gy09535 commented Apr 7, 2022 •

edited

Loading

codecov-commenter commented Apr 7, 2022 •

edited

Loading

nobodyiam Apr 8, 2022

gy09535 Apr 8, 2022

nobodyiam Apr 8, 2022

gy09535 Apr 8, 2022 •

edited

Loading

nobodyiam Apr 9, 2022

gy09535 Apr 11, 2022

nobodyiam Apr 8, 2022 •

edited

Loading

gy09535 Apr 8, 2022

Anilople commented Apr 8, 2022

nobodyiam commented Apr 8, 2022

nobodyiam Apr 8, 2022

gy09535 Apr 11, 2022

nobodyiam Apr 9, 2022

Anilople commented Apr 9, 2022

stale bot commented May 15, 2022

nobodyiam commented May 15, 2022

gy09535 commented Jun 14, 2022

nobodyiam commented Jun 15, 2022

feat: Connect spring cloud properties refresh #4303

feat: Connect spring cloud properties refresh #4303

Conversation

gy09535 commented Apr 7, 2022 • edited Loading

What's the purpose of this PR

参考：

Which issue(s) this PR fixes:

codecov-commenter commented Apr 7, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gy09535 Apr 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nobodyiam Apr 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Anilople commented Apr 8, 2022

nobodyiam commented Apr 8, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Anilople commented Apr 9, 2022

stale bot commented May 15, 2022

nobodyiam commented May 15, 2022

gy09535 commented Jun 14, 2022

nobodyiam commented Jun 15, 2022

gy09535 commented Apr 7, 2022 •

edited

Loading

codecov-commenter commented Apr 7, 2022 •

edited

Loading

gy09535 Apr 8, 2022 •

edited

Loading

nobodyiam Apr 8, 2022 •

edited

Loading