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

Broadcast ConfigChangeEvent using Spring ApplicationEvent #4305

Merged

Conversation

nobodyiam
Copy link
Member

@nobodyiam nobodyiam commented Apr 9, 2022

What's the purpose of this PR

Broadcast ConfigChangeEvent using Spring ApplicationEvent so that it's easy to customize the logic to handle config changes.

Brief changelog

  • Add ApolloConfigChangeEvent class
  • Refactor PropertySourcesProcessor to publish ApolloConfigChangeEvent when config changes

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
Copy link

codecov-commenter commented Apr 9, 2022

Codecov Report

Merging #4305 (343fcf9) into master (a42b793) will increase coverage by 0.03%.
The diff coverage is 100.00%.

❗ Current head 343fcf9 differs from pull request most recent head 7232497. Consider uploading reports for the commit 7232497 to get more accurate results

@@             Coverage Diff              @@
##             master    #4305      +/-   ##
============================================
+ Coverage     53.17%   53.21%   +0.03%     
- Complexity     2668     2673       +5     
============================================
  Files           488      489       +1     
  Lines         15253    15267      +14     
  Branches       1577     1576       -1     
============================================
+ Hits           8111     8124      +13     
  Misses         6587     6587              
- Partials        555      556       +1     
Impacted Files Coverage Δ
...apollo/spring/config/PropertySourcesProcessor.java 96.49% <100.00%> (+0.12%) ⬆️
...pring/property/AutoUpdateConfigChangeListener.java 92.72% <100.00%> (+0.72%) ⬆️
...ork/apollo/spring/spi/ApolloConfigChangeEvent.java 100.00% <100.00%> (ø)
...spring/spi/DefaultApolloConfigRegistrarHelper.java 75.75% <100.00%> (+1.56%) ⬆️
...i/DefaultConfigPropertySourcesProcessorHelper.java 100.00% <100.00%> (ø)
...ervice/service/ReleaseMessageServiceWithCache.java 84.70% <0.00%> (-1.18%) ⬇️

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 a42b793...7232497. Read the comment docs.

@misselvexu
Copy link
Contributor

Why not consider implementing the configuration auto-refresh mechanism based on EnvironmentChangeEvent ?

@nobodyiam
Copy link
Member Author

Why not consider implementing the configuration auto-refresh mechanism based on EnvironmentChangeEvent ?

This is an interesting idea.
The reason why we don't use EnvironmentChangeEvent is the refactored piece of codes should support all Spring scenarios, not just Spring Cloud.
However, one thing that might make sense is we use the ApplicationEvent mechanism to publish the config changes, e.g. publisher.publishEvent(springConfigChangeEvent), which would be more natural in Spring environment.

@misselvexu
Copy link
Contributor

Why not consider implementing the configuration auto-refresh mechanism based on EnvironmentChangeEvent ?

This is an interesting idea. The reason why we don't use EnvironmentChangeEvent is the refactored piece of codes should support all Spring scenarios, not just Spring Cloud. However, one thing that might make sense is we use the ApplicationEvent mechanism to publish the config changes, e.g. publisher.publishEvent(springConfigChangeEvent), which would be more natural in Spring environment.

Thank you for your answer, that whether to consider separating the client sdk, such as java version, spring version, spring boot version, spring cloud and so on version of the sdk.

@nobodyiam
Copy link
Member Author

Why not consider implementing the configuration auto-refresh mechanism based on EnvironmentChangeEvent ?

This is an interesting idea. The reason why we don't use EnvironmentChangeEvent is the refactored piece of codes should support all Spring scenarios, not just Spring Cloud. However, one thing that might make sense is we use the ApplicationEvent mechanism to publish the config changes, e.g. publisher.publishEvent(springConfigChangeEvent), which would be more natural in Spring environment.

Thank you for your answer, that whether to consider separating the client sdk, such as java version, spring version, spring boot version, spring cloud and so on version of the sdk.

We already had such discussions that ultimately there might be 3 java SDKs: apollo-client, apollo-client-config-data, spring-cloud-apollo-client(this name is not finalized).

@nobodyiam nobodyiam force-pushed the refactor-spring-change-listener branch 3 times, most recently from 7232497 to e33eb58 Compare April 16, 2022 03:17
@nobodyiam nobodyiam changed the title enable SpringConfigChangeListener mechanism Broadcast ConfigChangeEvent using Spring ApplicationEvent Apr 16, 2022
@nobodyiam nobodyiam force-pushed the refactor-spring-change-listener branch from e33eb58 to 6ed574c Compare April 16, 2022 03:24
@nobodyiam
Copy link
Member Author

@misselvexu @gy09535

I have updated the logic to broadcast the config change event using Spring ApplicationEvent, which should be more natural and easy to customize. Please help to take a look.

@nobodyiam nobodyiam added this to the 2.0.0 milestone Apr 17, 2022
@nobodyiam nobodyiam force-pushed the refactor-spring-change-listener branch from 6ed574c to 41f1189 Compare April 17, 2022 03:35
@nobodyiam nobodyiam marked this pull request as ready for review April 17, 2022 03:37
@nobodyiam nobodyiam requested review from a team and removed request for a team April 17, 2022 03:38
@nobodyiam nobodyiam requested a review from Anilople April 22, 2022 00:12
Copy link
Contributor

@lepdou lepdou left a comment

Choose a reason for hiding this comment

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

LGTM

@lepdou lepdou merged commit 49028e8 into apolloconfig:master Apr 22, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Apr 22, 2022
@nobodyiam nobodyiam deleted the refactor-spring-change-listener branch April 23, 2022 01:21
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants