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.
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
Support dynamic update cache config #13679
Changes from 12 commits
a541c4c
515d3f2
1847e9e
9e07e9a
562e7a9
e4c9c89
638139c
c3cc4a7
5e7e36d
bc80b0e
0eb2ee2
8954714
091fd88
c5c8a1f
a27d889
c34cf73
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.