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

Refactor ConfigurationChangeDetector to simplify conditionals. Issue 643 #644

Merged
merged 6 commits into from
Oct 1, 2020

Conversation

krisiye
Copy link
Contributor

@krisiye krisiye commented Sep 26, 2020

…ge detectors each for reload detection mode event/polling. Fixes #643

  1. Adds a changeDetector each for configMaps/Secrets and event/polling modes
  2. Moves reload mode conditionals into a sub package, usable by both autoConfiguration and defaults
  3. Additional tests
  4. updates to logging to indicate event/polling watcher activation

@spencergibb spencergibb changed the title Issue 643 Refactor ConfigurationChangeDetector to simplify conditionals. Issue 643 Sep 28, 2020
@krisiye
Copy link
Contributor Author

krisiye commented Sep 28, 2020

@ryanjbaxter - This is an attempt to refactor the configuration watcher implementation. It also sounds like based on the docs, the watcher as a side-car is the recommended approach as opposed to the native reload mechanism that is deprecated (wonder why? would benefit from some more background on this.) . May be best to also update the docs to reflect this and make it more obvious in the context of all of the reload config options available.

@ryanjbaxter
Copy link
Contributor

@krisiye wow! Thanks for this! Amazing work!

The reasons for the controller based approach is because using the Kubernetes API by applications running on Kubernetes is generally discouraged. Most of what spring-cloud-kubernetes-config did was read ConfigMaps and Secrets from Kubernetes and creating PropertySources from them. That piece of functionality can easily be replaced by mounting the ConfigMaps and Secrets as volumes. The piece of functionality that is missing when mounting them as volumes is notifying the app that the configuration changed and should be refreshed. This is where the new controller comes in. It is the one watching the ConfigMaps and Secrets for changes and then uses Spring Cloud Bus to notify the app of the changes.

Does that make sense?

@krisiye
Copy link
Contributor Author

krisiye commented Sep 29, 2020

Thanks @ryanjbaxter That makes a lot of sense! Decoupling from app and running as a sidecar seems more scalable! For the bus implementation, It may also make sense to support other messaging implementations in the future. For example: kafka

@ryanjbaxter
Copy link
Contributor

@krisiye yes, plan is to support everything bus supports! (Again, if you have any interest in helping out there we would appreciate it!). Could you fix the merge conflicts and update the PR?

@krisiye
Copy link
Contributor Author

krisiye commented Sep 30, 2020

@ryanjbaxter - Fixed conflicts. Should be good to go. CI is failing for unrelated modules looking for dependencies. Possible to re-run? I am unable to re-run on my end. missing permissions.

@krisiye
Copy link
Contributor Author

krisiye commented Sep 30, 2020

@ryanjbaxter The watcher (controller) also needs some improved logging both info and debug. I'll work on those and log issues.

Copy link
Contributor

@ryanjbaxter ryanjbaxter left a comment

Choose a reason for hiding this comment

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

This looks good to me. I also did some testing on my own and everything seemed to work.

@spencergibb @Haybu if you have a second could you take a look as well?

@ryanjbaxter ryanjbaxter merged commit a4ad5eb into spring-cloud:master Oct 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor ConfigurationChangeDetector to simplify conditionals
4 participants