CASSANDRA-18352: Bound user provided timestamps via guardrail #2246
Conversation
jrwest
force-pushed
the
jwest/18352
branch
2 times, most recently
from
979585d to
acbcbe4
Compare
test/unit/org/apache/cassandra/db/guardrails/GuardrailMaximumTimestampTest.java
Outdated
Show resolved
Hide resolved
test/unit/org/apache/cassandra/db/guardrails/GuardrailMinimumTimestampTest.java
Outdated
Show resolved
Hide resolved
| validateTimestampThreshold(config.maximum_timestamp_warn_threshold, | ||
| config.maximum_timestamp_fail_threshold, "maximum_timestamp"); |
There was a problem hiding this comment.
This isn't validating the new duration argument, but the previous values. So the following test will wrongly pass:
guardrails().setMaximumTimestampFailThreshold(1);
guardrails().setMaximumTimestampWarnThreshold(2);Same for the other three setters. Also, there isn't any test verifying the behaviour of this validation.
conf/cassandra.yaml
Outdated
| @@ -1757,6 +1757,12 @@ drop_compact_storage_enabled: false | |||
| # Guardrail to allow/disallow user-provided timestamps. Defaults to true. | |||
| # user_timestamps_enabled: true | |||
| # | |||
| # Guardrail to bound user-provided timestamps within a given range. Default is infinite (denoted by 0) | |||
There was a problem hiding this comment.
I think the convention for disables duration, sizes, etc. is that the disabled value is null instead of zero.
Note also that, even if we were considering zero as the disabled value, the following config will wrongly fail validation:
guardrails().setMaximumTimestampFailThreshold(0);
guardrails().setMaximumTimestampWarnThreshold(1);That would fail because 0 is not consistently considered as the disabled value.
I think we should use null as the disabled value for the DurationSpec properties. The int values in seconds that we use on the JMX methods can instead use -1 as their disabled value. More or less this way.
| * | ||
| * @param durationSeconds currentTimestamp + TimeUnit.SECONDS.toMicros(duration) | ||
| */ | ||
| void setMaximumTimestampWarnThreshold(int durationSeconds); |
There was a problem hiding this comment.
During the initial review for guardrails we decided to use a common setter method for both the warn and fail threshold. IIRC the reason was that the requisite of keeping the warn threshold lesser than the fail threshold imposes a certain order on how the separate setter would need to be called. For example, consider the following:
guardrails().setMinimumTimestampWarnThreshold(1);
guardrails().setMinimumTimestampFailThreshold(2);That might fail or not fail depending on the previous value of the fail threshold. However, the combined setter will always succeed:
guardrails().setMinimumTimestampThreshold(1, 2);I think we should keep the current style of combined setters in GuardrailsMBean for the sake of consistency.
There was a problem hiding this comment.
Will change to use a single setter
| * @param currentTimestamp the current timestamp | ||
| * @return The highest acceptable timestamp before triggering a failure | ||
| */ | ||
| long getMaximumTimestampFailThreshold(long currentTimestamp); |
There was a problem hiding this comment.
Probably it would make sense to have JMX getters returning the value of the property as it appears in config, instead of the ones relative to the current timestamp. Those getters would be the counterpart of the setters, which seem to accept the yaml property as argument.
There was a problem hiding this comment.
Perhaps we can try to homogenize the JMX methods and config so they only deal with the config properties as they are expressed in yaml, and encapsulate in Guardrails.java the part about making those thresholds relative to the current time. More or less this way. wdyt?
There was a problem hiding this comment.
I think the challenge here is it will fail JMXStandardsTest
There was a problem hiding this comment.
Ah I think I see in your patch you may have addressed that. Will take a closer look.
| /** | ||
| * Sets the failure upper bound for user supplied timestamps | ||
| * | ||
| * @param durationSeconds currentTimestamp + TimeUnit.SECONDS.toMicros(duration) |
There was a problem hiding this comment.
I think the setters accept the config duration alone, as defined in the yaml, without adding the current timestamp.
jrwest
force-pushed
the
jwest/18352
branch
2 times, most recently
from
81b54be to
37d33c6
Compare
| @@ -421,6 +424,24 @@ public final class Guardrails implements GuardrailsMBean | |||
| format("The keyspace %s has a replication factor of %s, above the %s threshold of %s.", | |||
| what, value, isWarning ? "warning" : "failure", threshold)); | |||
|
|
|||
| public static final MaxThreshold maximumAllowableTimestamp = | |||
| new MaxThreshold("maximum_timestamp", | |||
| null, | |||
There was a problem hiding this comment.
The optional "reason" argument was added quite recently, so most guardrails don't use it. In fact, the only guardrails that use it are allowFilteringEnabled and zeroTTLOnTWCSEnabled. That argument is meant to provide the reason for having such a guardrail, so it can be included in the warn/fail messages.
Maybe we could add here something like Timestamps too far in the future are not realistic, or something like that, if that makes sense. wdyt?
jrwest
force-pushed
the
jwest/18352
branch
2 times, most recently
from
75bbb88 to
24a071c
Compare
…a certain range) Patch by Jordan West; Reviewed by ----- for CASSANDRA-18352
No description provided.