You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by GitBox <gi...@apache.org> on 2022/11/22 15:03:56 UTC
[GitHub] [cassandra] adelapena commented on a diff in pull request #2025: CASSANDRA-18042 - WIP DO NOT COMMIT
adelapena commented on code in PR #2025:
URL: https://github.com/apache/cassandra/pull/2025#discussion_r1029424078
##########
src/java/org/apache/cassandra/db/guardrails/EnableFlag.java:
##########
@@ -33,6 +33,7 @@
public class EnableFlag extends Guardrail
{
private final Predicate<ClientState> enabled;
+ private final Predicate<ClientState> warned;
Review Comment:
All the other guardrails mention first the soft limit and then the hard limit. We could follow the same criteria here and reverse the order of the `enabled` and `warned` attributes. Same for the constructor arguments.
##########
test/unit/org/apache/cassandra/db/guardrails/GuardrailsTest.java:
##########
@@ -452,6 +453,22 @@ public void testPredicatesUsers() throws Throwable
assertValid(() -> guard.guard(101, superClientState));
}
+ @Test
+ public void testEnableFlagWarn() throws Throwable
Review Comment:
I'd put this test method immediately below `testEnableFlag` and `testEnableFlagUsers`, to keep related things together.
##########
test/unit/org/apache/cassandra/db/guardrails/GuardrailZeroDefaultTtlOnTimeWindowCompactionsStrategyTest.java:
##########
@@ -0,0 +1,67 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.cassandra.db.guardrails;
+
+import org.junit.Test;
+
+public class GuardrailZeroDefaultTtlOnTimeWindowCompactionsStrategyTest extends GuardrailTester
Review Comment:
Most instances of `GuardrailTester` have a `testExcludedUsers` test method to verify behaviour for excluded users. We could do the same here:
```java
@Test
public void testExcludedUsers() throws Throwable
{
for (boolean enabled : new boolean[]{ false, true })
{
setGuardrail(enabled);
testExcludedUsers(() -> QUERY,
() -> VALID_QUERY_1,
() -> VALID_QUERY_2);
}
}
```
##########
test/unit/org/apache/cassandra/db/guardrails/GuardrailsTest.java:
##########
@@ -452,6 +453,22 @@ public void testPredicatesUsers() throws Throwable
assertValid(() -> guard.guard(101, superClientState));
}
+ @Test
+ public void testEnableFlagWarn() throws Throwable
Review Comment:
Since the new warn was producing duplicated warnings for `null` client states, I'd consider that case in this test:
```java
@Test
public void testEnableFlagWarn() throws Throwable
{
EnableFlag disabledGuard = new EnableFlag("x", null, state -> false, state -> true, FEATURE);
assertFails(() -> disabledGuard.ensureEnabled(null), false, FEATURE + " is not allowed");
assertFails(() -> disabledGuard.ensureEnabled(userClientState), FEATURE + " is not allowed");
assertValid(() -> disabledGuard.ensureEnabled(systemClientState));
assertValid(() -> disabledGuard.ensureEnabled(superClientState));
EnableFlag enabledGuard = new EnableFlag("x", null, state -> true, state -> true, FEATURE);
assertWarns(() -> enabledGuard.ensureEnabled(null), FEATURE);
assertWarns(() -> enabledGuard.ensureEnabled(userClientState), FEATURE);
assertValid(() -> disabledGuard.ensureEnabled(systemClientState));
assertValid(() -> disabledGuard.ensureEnabled(superClientState));
}
```
##########
src/java/org/apache/cassandra/db/guardrails/Guardrails.java:
##########
@@ -199,6 +200,19 @@ public final class Guardrails implements GuardrailsMBean
state -> CONFIG_PROVIDER.getOrCreate(state).getCompactTablesEnabled(),
"Creation of new COMPACT STORAGE tables");
+ /**
+ * Guardrail disabling the creation of a new table when default_time_to_live is set to 0 and
+ * compaction strategy is TimeWindowCompactionStrategy or its subclass. If this guardrail does not fail
+ * and such configuration combination is found, it will always warn.
+ */
Review Comment:
We could use JavaDoc links:
```suggestion
/**
* Guardrail disabling the creation of a new table when {@link TableParams.Option#DEFAULT_TIME_TO_LIVE} is set to 0
* and compaction strategy is {@link TimeWindowCompactionStrategy} or its subclass. If this guardrail does not fail
* and such configuration combination is found, it will always warn.
*/
```
##########
src/java/org/apache/cassandra/db/guardrails/EnableFlag.java:
##########
@@ -95,5 +120,8 @@ public void ensureEnabled(String featureName, @Nullable ClientState state)
{
if (!isEnabled(state))
fail(featureName + " is not allowed", state);
Review Comment:
Note that `GuardrailFail` can emit warnings without throwing an exception. That happens when the `ClientState` is unknown, and it's used for background processes. So we need to either add a `return` after the call to `fail`, or put the warning part in an `else` branch. For example:
```java
if (!isEnabled(state))
{
fail(featureName + " is not allowed", state);
return;
}
```
##########
src/java/org/apache/cassandra/db/guardrails/EnableFlag.java:
##########
@@ -95,5 +120,8 @@ public void ensureEnabled(String featureName, @Nullable ClientState state)
{
if (!isEnabled(state))
fail(featureName + " is not allowed", state);
Review Comment:
Also, we can avoid the duplicated checks of the `ClientState` by just checking it once at the beginning of the method:
```java
public void ensureEnabled(String featureName, @Nullable ClientState state)
{
if (!enabled(state))
return;
if (!enabled.test(state))
{
fail(featureName + " is not allowed", state);
return;
}
if (warned.test(state))
warn(featureName);
}
```
##########
src/java/org/apache/cassandra/db/guardrails/Guardrails.java:
##########
@@ -199,6 +200,19 @@ public final class Guardrails implements GuardrailsMBean
state -> CONFIG_PROVIDER.getOrCreate(state).getCompactTablesEnabled(),
"Creation of new COMPACT STORAGE tables");
+ /**
+ * Guardrail disabling the creation of a new table when default_time_to_live is set to 0 and
+ * compaction strategy is TimeWindowCompactionStrategy or its subclass. If this guardrail does not fail
+ * and such configuration combination is found, it will always warn.
+ */
+ public static final EnableFlag zeroDefaultTTLonTimeWindowCompactionStrategyEnabled =
+ new EnableFlag("zero_default_ttl_on_time_window_compaction_strategy_enabled",
+ "It is suspicious to use default_time_to_live set to 0 with such compaction strategy. Please keep in mind " +
+ "that data will not start to automatically expire after they are older than a respective compaction window unit of a certain size. ",
Review Comment:
Line width:
```suggestion
new EnableFlag("zero_default_ttl_on_time_window_compaction_strategy_enabled",
"It is suspicious to use default_time_to_live set to 0 with such compaction strategy. Please keep " +
"in mind that data will not start to automatically expire after they are older than a respective " +
"compaction window unit of a certain size. ",
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org