CASSANDRA-19195 Added guardrail to enforce minimum CMS size#4950
CASSANDRA-19195 Added guardrail to enforce minimum CMS size#4950nvharikrishna wants to merge 2 commits into
Conversation
| private void guardMinimumCmsSize(ReplicationParams params) | ||
| { | ||
| ClusterMetadata metadata = ClusterMetadata.current(); | ||
| int minCmsSize = DatabaseDescriptor.getGuardrailsConfig().getMinimumCmsSizeFailThreshold(); |
There was a problem hiding this comment.
Interesting usage. I would use MinThreshold API here and called Guardrails.instance.minimumCmsSize.failValue|warnValue instead. We would then introduce CMSSizeThreshold guardrail class extending MinThreshold with overridden methods.
Or at least getting int minCmsSize from Guardrails.instance + appropriate getter if above is overkill.
There was a problem hiding this comment.
Thanks for the suggestion! I looked into the CMSSizeThreshold extends MinThreshold approach. This validation needs two different values: cmsSize - the actual value being guarded, and totalNodes - a precondition deciding whether the check should apply at all (skipping the enforcement when the cluster is smaller than the required CMS size). totalNodes is a live value derived from ClusterMetadata.current(). The Threshold API has no way to pass it in (enabled()/guard() only take ClientState), so a CMSSizeThreshold subclass would have to reach into the ClusterMetadata.current() singleton from inside guard() just to get it. That would invert our package dependencies — db.guardrails currently imports nothing from tcm, and this would make a guardrail depend on cluster metadata. I feel there isn't much to reuse from the parent.
I opted for the second suggestion; it’s simpler. Now, both the threshold lookup and enforcement pass through Guardrails. I already have a getter for it and have used it here.
There was a problem hiding this comment.
@nvharikrishna you could do it the same way as in ClientDriverVersionGuardrail which introduces a custom guard method accepting additional values so there you would pass values from CMS you would obtain outside of that guardrail. You would not need to depend on CMS inside guardrail.
There was a problem hiding this comment.
@nvharikrishna check this https://github.com/apache/cassandra/compare/trunk...smiklosovic:cassandra:CASSANDRA-19195-cms-guardrail-trunk?expand=1
What I like about that approach is that it encapsulates that logic and all guardrail's construction etc. It is just easier for an eye.
Adds the minimum_cms_size_fail_threshold guardrail that fails an operator-requested CMS reconfiguration (nodetool cms reconfigure / JMX) when the resulting CMS size (aggregate replication factor across DCs) would fall below the threshold. - Config-driven, default -1 (disabled); valid values are -1 or >= 3 - Enforced only on operator-initiated reconfiguration, not the automatic reconfiguration on topology changes - Skipped for clusters smaller than the threshold - Fail-only (no warning threshold) patch by nvharikrishna; reviewed by <reviewer> for CASSANDRA-19195
422e2c1 to
e1e5c53
Compare
Adds the minimum_cms_size_fail_threshold guardrail that fails an operator-requested CMS reconfiguration (nodetool cms reconfigure / JMX) when the resulting CMS size (aggregate replication factor across DCs) would fall below the threshold.
patch by nvharikrishna; reviewed by <reviewer> for CASSANDRA-19195
The CASSANDRA-19195