[GitHub] [cassandra] jonmeredith commented on a diff in pull request #1532: CASSANDRA-17495

[GitBox <gi...@apache.org>]

jonmeredith commented on code in PR #1532:
URL: https://github.com/apache/cassandra/pull/1532#discussion_r850728352


##########
test/unit/org/apache/cassandra/db/guardrails/GuardrailAlterTableTest.java:
##########
@@ -56,29 +56,29 @@ public void testAlterTableGuardrail() throws Throwable
         setGuardrail(true);
         assertValid("ALTER TABLE %s WITH compression = { 'class' : 'SnappyCompressor', 'chunk_length_in_kb' : 32 };");
 
-        // Confirm works when disabled
+        // Confirm guardrail works when disabled
         setGuardrail(false);
-        assertValid("ALTER TABLE %s ADD test_one text;");
+        assertFails("ALTER TABLE %s ADD test_one text;", "changing columns");
 
-        // Confirm fails when enabled
+        // Confirm works when enabled
         setGuardrail(true);
-        assertFails("ALTER TABLE %s ADD test_two text;", "changing columns");
+        assertValid("ALTER TABLE %s ADD test_two text;");
 
-        // Confirm works again when disabled
+        // Confirm fails again when disabled
         setGuardrail(false);
-        assertValid("ALTER TABLE %s ADD test_three text;");
+        assertFails("ALTER TABLE %s ADD test_three text;", "changing columns");
 
-        // Fail to ALTER-DROP
-        setGuardrail(true);
+        // Confirm stops ALTER + DROP columns
+        setGuardrail(false);
         assertFails("ALTER TABLE %s DROP test_one", "changing columns");
 
-        // Fail to ALTER-RENAME
+        // Confirm stops ALTER + RENAME columns
         assertFails("ALTER TABLE %s RENAME test_one TO test_ren_one", "changing columns");
 
-        // Allow ALTER table options even when alter table is disabled
+        // Allow ALTER table *options* works even when alter table is disabled
         assertValid("ALTER TABLE %s WITH GC_GRACE_SECONDS = 123; ");
 
-        // Restore things so they work for other tests
+        // Restore things so they work for other tests that may be added to this class in the future
         setGuardrail(true);

Review Comment:
   Maybe better in an @After or in a `try/finally` block?



##########
test/unit/org/apache/cassandra/db/guardrails/GuardrailAlterTableTest.java:
##########
@@ -56,29 +56,29 @@ public void testAlterTableGuardrail() throws Throwable
         setGuardrail(true);
         assertValid("ALTER TABLE %s WITH compression = { 'class' : 'SnappyCompressor', 'chunk_length_in_kb' : 32 };");
 
-        // Confirm works when disabled
+        // Confirm guardrail works when disabled

Review Comment:
   All this negative and double-negative stuff is ripe for confusion. Comment makes it read that the guardrail works, not the statement. Maybe `Confirm ALTER TABLE forbidden when alter_table_enabled is false` is more explicit.



-- 
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