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 dynamic update cache config #13679
Merged
eolivelli
merged 16 commits into
apache:master
from
lordcheng10:support_dynamic_cache_config
Mar 14, 2022
Merged
Changes from all commits
Commits
Show all changes
16 commits
Select commit
Hold shift + click to select a range
a541c4c
use registerConfigurationListener for dynamic update cache configs
lordcheng10 515d3f2
check style
lordcheng10 1847e9e
check style
lordcheng10 9e07e9a
check style
lordcheng10 562e7a9
remove new config : evictionTriggerThresholdPercent
lordcheng10 e4c9c89
check style
lordcheng10 638139c
remove change for evictionTriggerThresholdPercent
lordcheng10 c3cc4a7
rename cacheEvictionTimeThresholdMills
lordcheng10 5e7e36d
move updateCacheSizeAndThreshold into method in ManagedLedgerFactor…
lordcheng10 bc80b0e
check style
lordcheng10 0eb2ee2
check style
lordcheng10 8954714
check style
lordcheng10 091fd88
remove updateCacheSizeAndThreshold
lordcheng10 c5c8a1f
add unit test
lordcheng10 a27d889
1.rename getCacheEvictionTimeThresholdNanos;2.use Awaiatility
lordcheng10 c34cf73
check style
lordcheng10 File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am concerned about the cost of this change. These three variables are read each time that a message is written to the cache. While I see the generic benefit of increasing configurability, I don't think we should do so in a way that adds cost (even small costs) to the write path. @lordcheng10 - do you have a use case that shows why the benefits out way the costs here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@merlimat - do you have any thoughts here? I remember that you have raised concerns about
volatile
in the metadatastore.I know that we are talking about minor optimizations here, but since this is on the write path, I feel it makes sense to discuss now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we need to adjust the cache parameters, we need to restart the online cluster in turn. Restarting the cluster will have a certain impact on online business.
For different business scenarios, we cannot give a more reasonable cache configuration at once. May need to adjust the cache configuration multiple times
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have some good ideas? Regarding the need to provide a dynamic configuration cache, but also to solve the problems you mentioned.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lordcheng10 - thanks for the additional context. It would be interesting to see a flame graph for the write path of the broker to identify hot spots. The only reason this PR caught my attention is because of the general heuristic that I've seen @merlimat employ: avoid
volatile
when possible. Without actual metrics about performance impact, I think we should leave this PR in place.