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

Improve the documentation for the config-service.cache.enabled configuration property. #4810

Merged
merged 81 commits into from
Mar 23, 2023

Conversation

klboke
Copy link
Contributor

@klboke klboke commented Mar 20, 2023

What's the purpose of this PR

Improve the documentation for the config-service.cache.enabled configuration property.

Motivation

  • We encountered a situation in our production environment where, when caching was enabled, we were unable to retrieve the latest configuration due to a case mismatch between apollo.cluster keys.

Possible solutions

If we want to enable in-memory caching and ensure that the loading behavior remains consistent with when it is not enabled, could we simply convert the cache key of Guava Cache to lowercase? Would this cause any additional problems? I'm not entirely sure. One thing to consider is that the code already handles namespaces in a similar way, so is this new issue consistent with that?

klboke and others added 30 commits May 16, 2019 11:07
@klboke klboke requested a review from nobodyiam March 20, 2023 12:51
@codecov
Copy link

codecov bot commented Mar 21, 2023

Codecov Report

Merging #4810 (bf5e812) into master (a441b6b) will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master    #4810   +/-   ##
=========================================
  Coverage     47.23%   47.23%           
  Complexity     1660     1660           
=========================================
  Files           346      346           
  Lines         10685    10685           
  Branches       1065     1065           
=========================================
  Hits           5047     5047           
  Misses         5329     5329           
  Partials        309      309           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@nobodyiam
Copy link
Member

If we want to enable in-memory caching and ensure that the loading behavior remains consistent with when it is not enabled, could we simply convert the cache key of Guava Cache to lowercase?

It's actually a known issue as is also recorded in the apollo project. There was an attempt to fix this issue #3542 but not continued. I think we could still consider fixing it if there is an easier way.
image

@klboke
Copy link
Contributor Author

klboke commented Mar 21, 2023

The worst part is that MySQL was not strictly set to match case sensitivity from the beginning, so now we have to do compatibility processing. I think we can merge this PR first, As for the compatibility processing issue, I will look into it later.

Copy link
Member

@nobodyiam nobodyiam left a comment

Choose a reason for hiding this comment

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

LGTM

@nobodyiam nobodyiam merged commit e11854f into apolloconfig:master Mar 23, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Mar 23, 2023
@nobodyiam nobodyiam added this to the 2.2.0 milestone Aug 23, 2023
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.

2 participants