-
Notifications
You must be signed in to change notification settings - Fork 3.8k
17737 4.0 #1748
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
17737 4.0 #1748
Conversation
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.
As in CASSANDRA-17735, I think I would remove GCInspector's local copies of Config's gc_log_threshold_in_ms and gc_warn_threshold_in_ms. Keeping those properties in a single place should easy synchronization.
For example, GCInspector#gcLogThreshholdInMs and GCInspector#gcWarnThresholdInMs are marked as volatile, so changes to these properties via JMX should be visible for other JMX readers and GCInspector itself. However, Config#gc_log_threshold_in_ms and Config#gc_warn_threshold_in_ms aren't marked as volatile. Thus, changes done to those properties through JMX could not be visible for the settings virtual table. Conversely, we could see the opposite situation when we have writable virtual tables.
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.
So what you suggest is just to make them volatile also in Config? And the long-int issue is already solved in the setter
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.
Setting them as volatile in Config should work, but I was thinking on removing the local copies on GCInspector, this way, as we did in CASSANDRA-17735. wdyt?
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.
Yes, sorry, I was not clear - that is exactly what I meant, making those in Config volatile further to removing the local copies
b4875c6 to
c25881a
Compare
…e JMX methods or directly Config class as now - we will have to decide whether to move the setters checks to the DD setters or not Fix conf.index_summary_capacity_in_mb Fix non-mutable prepared_statements_cache_size_mb, key_cache_size_in_mb, counter_cache_size_in_mb Remove local copies of config properties from GCInspector
c25881a to
b222e77
Compare
|
Done |
1 similar comment
|
Done |
### What is the issue Fixes riptano/cndb#14171 ### What does this PR fix and why was it fixed datastax#1200 introduced a bug for SAI indexes version AA that have clustering columns. As the tests show, updates incorrectly removed rows from the index. We need the update logic for later versions of SAI, so it is key to keep the update feature, but AA does not support those features precisely because it only indexes the partition key, so this is a safe update.
### What is the issue Fixes riptano/cndb#14171 ### What does this PR fix and why was it fixed datastax#1200 introduced a bug for SAI indexes version AA that have clustering columns. As the tests show, updates incorrectly removed rows from the index. We need the update logic for later versions of SAI, so it is key to keep the update feature, but AA does not support those features precisely because it only indexes the partition key, so this is a safe update.
No description provided.