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/21 10:32:37 UTC
[GitHub] [cassandra] smiklosovic opened a new pull request, #2025: CASSANDRA-18042 - WIP DO NOT COMMIT
smiklosovic opened a new pull request, #2025:
URL: https://github.com/apache/cassandra/pull/2025
Thanks for sending a pull request! Here are some tips if you're new here:
* Ensure you have added or run the [appropriate tests](https://cassandra.apache.org/_/development/testing.html) for your PR.
* Be sure to keep the PR description updated to reflect all changes.
* Write your PR title to summarize what this PR proposes.
* If possible, provide a concise example to reproduce the issue for a faster review.
* Read our [contributor guidelines](https://cassandra.apache.org/_/development/index.html)
* If you're making a documentation change, see our [guide to documentation contribution](https://cassandra.apache.org/_/development/documentation.html)
Commit messages should follow the following format:
```
<One sentence description, usually Jira title or CHANGES.txt summary>
<Optional lengthier description (context on patch)>
patch by <Authors>; reviewed by <Reviewers> for CASSANDRA-#####
Co-authored-by: Name1 <email1>
Co-authored-by: Name2 <email2>
```
The [Cassandra Jira](https://issues.apache.org/jira/projects/CASSANDRA/issues/)
--
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
[GitHub] [cassandra] adelapena commented on a diff in pull request #2025: CASSANDRA-18042 - WIP DO NOT COMMIT
Posted by GitBox <gi...@apache.org>.
adelapena commented on code in PR #2025:
URL: https://github.com/apache/cassandra/pull/2025#discussion_r1029670406
##########
src/java/org/apache/cassandra/db/guardrails/Guardrails.java:
##########
@@ -199,6 +201,20 @@ 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 {@link 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.
+ */
+ public static final EnableFlag zeroDefaultTTLOnTWCSEnabled =
+ new EnableFlag("zero_default_ttl_on_time_window_compaction_strategy_enabled",
Review Comment:
This should be the name of the guardrail, not the name of the property. So either`zero_default_ttl_on_time_window_compaction_strategy` or `zero_default_ttl_on_twcs`
--
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
[GitHub] [cassandra] adelapena commented on a diff in pull request #2025: CASSANDRA-18042 - WIP DO NOT COMMIT
Posted by GitBox <gi...@apache.org>.
adelapena commented on code in PR #2025:
URL: https://github.com/apache/cassandra/pull/2025#discussion_r1030319225
##########
test/unit/org/apache/cassandra/db/guardrails/GuardrailZeroDefaultTTLOnTWCSTest.java:
##########
@@ -0,0 +1,83 @@
+/*
+ * 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;
+
+import org.apache.cassandra.db.compaction.TimeWindowCompactionStrategy;
+
+public class GuardrailZeroDefaultTTLOnTWCSTest extends GuardrailTester
+{
+ private static final String QUERY = "CREATE TABLE IF NOT EXISTS tb1 (k int PRIMARY KEY, a int, b int) " +
+ "WITH default_time_to_live = 0 " +
+ "AND compaction = {'class': 'TimeWindowCompactionStrategy', 'enabled': true }";
+
+ private static final String VALID_QUERY_1 = "CREATE TABLE IF NOT EXISTS tb2 (k int PRIMARY KEY, a int, b int) " +
+ "WITH default_time_to_live = 1 " +
+ "AND compaction = {'class': 'TimeWindowCompactionStrategy', 'enabled': true }";
+
+ private static final String VALID_QUERY_2 = "CREATE TABLE IF NOT EXISTS tb3 (k int PRIMARY KEY, a int, b int) " +
+ "WITH default_time_to_live = 0";
+
+ public GuardrailZeroDefaultTTLOnTWCSTest()
+ {
+ super(Guardrails.zeroDefaultTTLOnTWCSEnabled);
+ }
+
+ @Test
+ public void testGuardrailEnabled() throws Throwable
+ {
+ setGuardrail(true);
+ assertWarns(QUERY, "0 default_time_to_live on a table with " +
+ TimeWindowCompactionStrategy.class.getSimpleName() + " compaction strategy");
+ }
+
+ @Test
+ public void testGuardrailDisabled() throws Throwable
+ {
+ setGuardrail(false);
+ assertFails(QUERY, "0 default_time_to_live on a table with " +
+ TimeWindowCompactionStrategy.class.getSimpleName() + " compaction strategy");
Review Comment:
```suggestion
TimeWindowCompactionStrategy.class.getSimpleName() + " compaction strategy is not allowed");
```
--
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
[GitHub] [cassandra] smiklosovic commented on a diff in pull request #2025: CASSANDRA-18042 - WIP DO NOT COMMIT
Posted by GitBox <gi...@apache.org>.
smiklosovic commented on code in PR #2025:
URL: https://github.com/apache/cassandra/pull/2025#discussion_r1029824778
##########
test/unit/org/apache/cassandra/db/guardrails/GuardrailZeroDefaultTTLOnTWCSTest.java:
##########
@@ -0,0 +1,81 @@
+/*
+ * 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;
+
+import org.apache.cassandra.db.compaction.TimeWindowCompactionStrategy;
+
+public class GuardrailZeroDefaultTTLOnTWCSTest extends GuardrailTester
+{
+ private static final String QUERY = "CREATE TABLE IF NOT EXISTS tb1 (k int PRIMARY KEY, a int, b int) " +
+ "WITH default_time_to_live = 0 " +
+ "AND compaction = {'class': 'TimeWindowCompactionStrategy', 'enabled': true }";
+
+ private static final String VALID_QUERY_1 = "CREATE TABLE IF NOT EXISTS tb2 (k int PRIMARY KEY, a int, b int) " +
+ "WITH default_time_to_live = 1 " +
+ "AND compaction = {'class': 'TimeWindowCompactionStrategy', 'enabled': true }";
+
+ private static final String VALID_QUERY_2 = "CREATE TABLE IF NOT EXISTS tb3 (k int PRIMARY KEY, a int, b int) " +
+ "WITH default_time_to_live = 0";
+
+ public GuardrailZeroDefaultTTLOnTWCSTest()
+ {
+ super(Guardrails.zeroDefaultTTLOnTWCSEnabled);
+ }
+
+ @Test
+ public void testGuardrailEnabled() throws Throwable
+ {
+ setGuardrail(true);
+ assertWarns(QUERY, "0 default_time_to_live on a table with " + TimeWindowCompactionStrategy.class.getSimpleName() + " compaction strategy");
Review Comment:
As commented previously, this string is in fact `featureName` but I have left it here as a simple string. Extracting `featureName` from`EnableFlag` is quite tricky / not so straightforward. This whole testing framework works with `Guardrail` so I would need to cast it etc etc. Maybe some getter EnableFlag for featureName as it is private etc ...
--
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
[GitHub] [cassandra] smiklosovic commented on a diff in pull request #2025: CASSANDRA-18042 - WIP DO NOT COMMIT
Posted by GitBox <gi...@apache.org>.
smiklosovic commented on code in PR #2025:
URL: https://github.com/apache/cassandra/pull/2025#discussion_r1029483187
##########
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:
ok, I was looking at that alphabetically.
--
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
[GitHub] [cassandra] smiklosovic closed pull request #2025: CASSANDRA-18042 - WIP DO NOT COMMIT
Posted by GitBox <gi...@apache.org>.
smiklosovic closed pull request #2025: CASSANDRA-18042 - WIP DO NOT COMMIT
URL: https://github.com/apache/cassandra/pull/2025
--
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
[GitHub] [cassandra] adelapena commented on a diff in pull request #2025: CASSANDRA-18042 - WIP DO NOT COMMIT
Posted by GitBox <gi...@apache.org>.
adelapena commented on code in PR #2025:
URL: https://github.com/apache/cassandra/pull/2025#discussion_r1029674082
##########
test/unit/org/apache/cassandra/db/guardrails/GuardrailZeroDefaultTTLOnTWCSTest.java:
##########
@@ -0,0 +1,79 @@
+/*
+ * 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 GuardrailZeroDefaultTTLOnTWCSTest extends GuardrailTester
+{
+ private static final String QUERY = "CREATE TABLE IF NOT EXISTS tb1 (k int PRIMARY KEY, a int, b int) " +
+ "WITH default_time_to_live = 0 " +
+ "AND compaction = {'class': 'TimeWindowCompactionStrategy', 'enabled': true }";
+
+ private static final String VALID_QUERY_1 = "CREATE TABLE IF NOT EXISTS tb2 (k int PRIMARY KEY, a int, b int) " +
+ "WITH default_time_to_live = 1 " +
+ "AND compaction = {'class': 'TimeWindowCompactionStrategy', 'enabled': true }";
+
+ private static final String VALID_QUERY_2 = "CREATE TABLE IF NOT EXISTS tb3 (k int PRIMARY KEY, a int, b int) " +
+ "WITH default_time_to_live = 0";
+
+ public GuardrailZeroDefaultTTLOnTWCSTest()
+ {
+ super(Guardrails.zeroDefaultTTLOnTWCSEnabled);
+ }
+
+ @Test
+ public void testGuardrailEnabled() throws Throwable
+ {
+ setGuardrail(true);
+ assertWarns(QUERY, guardrail.reason);
+ }
+
+ @Test
+ public void testGuardrailDisabled() throws Throwable
+ {
+ setGuardrail(false);
+ assertFails(QUERY, guardrail.reason);
Review Comment:
The second argument should be the warn message, `0 default_time_to_live on a table with TimeWindowCompactionStrategy compaction strategy is not allowed`. The reason is already automatically checked by the test method.
--
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
[GitHub] [cassandra] adelapena commented on a diff in pull request #2025: CASSANDRA-18042 - WIP DO NOT COMMIT
Posted by GitBox <gi...@apache.org>.
adelapena commented on code in PR #2025:
URL: https://github.com/apache/cassandra/pull/2025#discussion_r1029674002
##########
test/unit/org/apache/cassandra/db/guardrails/GuardrailZeroDefaultTTLOnTWCSTest.java:
##########
@@ -0,0 +1,79 @@
+/*
+ * 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 GuardrailZeroDefaultTTLOnTWCSTest extends GuardrailTester
+{
+ private static final String QUERY = "CREATE TABLE IF NOT EXISTS tb1 (k int PRIMARY KEY, a int, b int) " +
+ "WITH default_time_to_live = 0 " +
+ "AND compaction = {'class': 'TimeWindowCompactionStrategy', 'enabled': true }";
+
+ private static final String VALID_QUERY_1 = "CREATE TABLE IF NOT EXISTS tb2 (k int PRIMARY KEY, a int, b int) " +
+ "WITH default_time_to_live = 1 " +
+ "AND compaction = {'class': 'TimeWindowCompactionStrategy', 'enabled': true }";
+
+ private static final String VALID_QUERY_2 = "CREATE TABLE IF NOT EXISTS tb3 (k int PRIMARY KEY, a int, b int) " +
+ "WITH default_time_to_live = 0";
+
+ public GuardrailZeroDefaultTTLOnTWCSTest()
+ {
+ super(Guardrails.zeroDefaultTTLOnTWCSEnabled);
+ }
+
+ @Test
+ public void testGuardrailEnabled() throws Throwable
+ {
+ setGuardrail(true);
+ assertWarns(QUERY, guardrail.reason);
Review Comment:
The second argument should be the warn message, `0 default_time_to_live on a table with TimeWindowCompactionStrategy compaction strategy`. The reason is already automatically checked by the test method.
##########
test/unit/org/apache/cassandra/db/guardrails/GuardrailZeroDefaultTTLOnTWCSTest.java:
##########
@@ -0,0 +1,79 @@
+/*
+ * 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 GuardrailZeroDefaultTTLOnTWCSTest extends GuardrailTester
+{
+ private static final String QUERY = "CREATE TABLE IF NOT EXISTS tb1 (k int PRIMARY KEY, a int, b int) " +
+ "WITH default_time_to_live = 0 " +
+ "AND compaction = {'class': 'TimeWindowCompactionStrategy', 'enabled': true }";
+
+ private static final String VALID_QUERY_1 = "CREATE TABLE IF NOT EXISTS tb2 (k int PRIMARY KEY, a int, b int) " +
+ "WITH default_time_to_live = 1 " +
+ "AND compaction = {'class': 'TimeWindowCompactionStrategy', 'enabled': true }";
+
+ private static final String VALID_QUERY_2 = "CREATE TABLE IF NOT EXISTS tb3 (k int PRIMARY KEY, a int, b int) " +
+ "WITH default_time_to_live = 0";
+
+ public GuardrailZeroDefaultTTLOnTWCSTest()
+ {
+ super(Guardrails.zeroDefaultTTLOnTWCSEnabled);
+ }
+
+ @Test
+ public void testGuardrailEnabled() throws Throwable
+ {
+ setGuardrail(true);
+ assertWarns(QUERY, guardrail.reason);
+ }
+
+ @Test
+ public void testGuardrailDisabled() throws Throwable
+ {
+ setGuardrail(false);
+ assertFails(QUERY, guardrail.reason);
Review Comment:
The second argument should be the warn message, `0 default_time_to_live on a table with TimeWindowCompactionStrategy compaction strategy`. The reason is already automatically checked by the test method.
--
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
[GitHub] [cassandra] adelapena commented on a diff in pull request #2025: CASSANDRA-18042 - WIP DO NOT COMMIT
Posted by GitBox <gi...@apache.org>.
adelapena commented on code in PR #2025:
URL: https://github.com/apache/cassandra/pull/2025#discussion_r1029676560
##########
src/java/org/apache/cassandra/db/guardrails/EnableFlag.java:
##########
@@ -93,7 +118,16 @@ public void ensureEnabled(@Nullable ClientState state)
*/
public void ensureEnabled(String featureName, @Nullable ClientState state)
{
- if (!isEnabled(state))
+ if (!enabled(state))
+ return;
+
+ if (!enabled.test(state))
+ {
fail(featureName + " is not allowed", state);
+ return;
+ }
+
+ if (warned.test(state))
+ warn(featureName);
Review Comment:
Why not add something similar to what we have on the call to `fail`? For example:
```suggestion
warn(featureName + " is not recommended");
```
--
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
[GitHub] [cassandra] smiklosovic commented on a diff in pull request #2025: CASSANDRA-18042 - WIP DO NOT COMMIT
Posted by GitBox <gi...@apache.org>.
smiklosovic commented on code in PR #2025:
URL: https://github.com/apache/cassandra/pull/2025#discussion_r1029813022
##########
test/unit/org/apache/cassandra/db/guardrails/GuardrailZeroDefaultTTLOnTWCSTest.java:
##########
@@ -0,0 +1,79 @@
+/*
+ * 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 GuardrailZeroDefaultTTLOnTWCSTest extends GuardrailTester
+{
+ private static final String QUERY = "CREATE TABLE IF NOT EXISTS tb1 (k int PRIMARY KEY, a int, b int) " +
+ "WITH default_time_to_live = 0 " +
+ "AND compaction = {'class': 'TimeWindowCompactionStrategy', 'enabled': true }";
+
+ private static final String VALID_QUERY_1 = "CREATE TABLE IF NOT EXISTS tb2 (k int PRIMARY KEY, a int, b int) " +
+ "WITH default_time_to_live = 1 " +
+ "AND compaction = {'class': 'TimeWindowCompactionStrategy', 'enabled': true }";
+
+ private static final String VALID_QUERY_2 = "CREATE TABLE IF NOT EXISTS tb3 (k int PRIMARY KEY, a int, b int) " +
+ "WITH default_time_to_live = 0";
+
+ public GuardrailZeroDefaultTTLOnTWCSTest()
+ {
+ super(Guardrails.zeroDefaultTTLOnTWCSEnabled);
+ }
+
+ @Test
+ public void testGuardrailEnabled() throws Throwable
+ {
+ setGuardrail(true);
+ assertWarns(QUERY, guardrail.reason);
Review Comment:
Interestingly enough, this string is `featureName` in EnableFlag if you check `ensureEnabled` more closely. So it would be nice if we can use that class property rather than copying it here. However, `guardrail` here is of type `Guardrail`, not `EnableFlag` so I would need to cast it. Do you think that is ok?
--
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
[GitHub] [cassandra] adelapena commented on a diff in pull request #2025: CASSANDRA-18042 - WIP DO NOT COMMIT
Posted by GitBox <gi...@apache.org>.
adelapena commented on code in PR #2025:
URL: https://github.com/apache/cassandra/pull/2025#discussion_r1030318911
##########
test/unit/org/apache/cassandra/db/guardrails/GuardrailZeroDefaultTTLOnTWCSTest.java:
##########
@@ -0,0 +1,83 @@
+/*
+ * 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;
+
+import org.apache.cassandra.db.compaction.TimeWindowCompactionStrategy;
+
+public class GuardrailZeroDefaultTTLOnTWCSTest extends GuardrailTester
+{
+ private static final String QUERY = "CREATE TABLE IF NOT EXISTS tb1 (k int PRIMARY KEY, a int, b int) " +
+ "WITH default_time_to_live = 0 " +
+ "AND compaction = {'class': 'TimeWindowCompactionStrategy', 'enabled': true }";
+
+ private static final String VALID_QUERY_1 = "CREATE TABLE IF NOT EXISTS tb2 (k int PRIMARY KEY, a int, b int) " +
+ "WITH default_time_to_live = 1 " +
+ "AND compaction = {'class': 'TimeWindowCompactionStrategy', 'enabled': true }";
+
+ private static final String VALID_QUERY_2 = "CREATE TABLE IF NOT EXISTS tb3 (k int PRIMARY KEY, a int, b int) " +
+ "WITH default_time_to_live = 0";
+
+ public GuardrailZeroDefaultTTLOnTWCSTest()
+ {
+ super(Guardrails.zeroDefaultTTLOnTWCSEnabled);
+ }
+
+ @Test
+ public void testGuardrailEnabled() throws Throwable
+ {
+ setGuardrail(true);
+ assertWarns(QUERY, "0 default_time_to_live on a table with " +
+ TimeWindowCompactionStrategy.class.getSimpleName() + " compaction strategy");
Review Comment:
```suggestion
TimeWindowCompactionStrategy.class.getSimpleName() + " compaction strategy is not recommended");
```
--
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
[GitHub] [cassandra] adelapena commented on a diff in pull request #2025: CASSANDRA-18042 - WIP DO NOT COMMIT
Posted by GitBox <gi...@apache.org>.
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