Add flag to disable ALTER...DROP COMPACT STORAGE statements

patch by Benjamin Lerer; review by Andres de la Pena and Brandon Williams for CASSANDRA-16733

CHANGES.txt

@@ -1,4 +1,5 @@
 3.0.25:
+ * Add flag to disable ALTER...DROP COMPACT STORAGE statements (CASSANDRA-16733)
  * Clean transaction log leftovers at the beginning of sstablelevelreset and sstableofflinerelevel (CASSANDRA-12519)
  * CQL shell should prefer newer TLS version by default (CASSANDRA-16695)
  * Ensure that existing empty rows are properly returned (CASSANDRA-16671)

NEWS.txt

@@ -42,6 +42,16 @@ restore snapshots created with the previous major version using the
 'sstableloader' tool. You can upgrade the file format of your snapshots
 using the provided 'sstableupgrade' tool.
 
+3.0.25
+======
+
+ALTER ... DROP COMPACT STORAGE
+------------------------------
+   - Following a discussion regarding concerns about the safety of the 'ALTER ... DROP COMPACT STORAGE' statement,
+     the C* development community does not recommend its use in production and considers it experimental
+     (see https://www.mail-archive.com/dev@cassandra.apache.org/msg16789.html).
+   - An 'enable_drop_compact_storage' flag has been added to cassandra.yaml to allow operators to prevent its use.
+
 3.0.24
 ======
 
@@ -139,7 +149,8 @@ Upgrading
 
 Compact Storage
 ---------------
-    - Starting version 4.0, Thrift and COMPACT STORAGE is no longer supported.
+    - Starting version 4.0, Thrift is no longer supported.
+      Starting version 5.0, COMPACT STORAGE will no longer be supported.
       'ALTER ... DROP COMPACT STORAGE' statement makes Compact Tables CQL-compatible,
       exposing internal structure of Thrift/Compact Tables. You can find more details
       on exposed internal structure under: 

conf/cassandra.yaml

@@ -989,6 +989,10 @@ enable_scripted_user_defined_functions: false
 # Materialized views are considered experimental and are not recommended for production use.
 enable_materialized_views: true
 
+# Enables the used of 'ALTER ... DROP COMPACT STORAGE' statements on this node.
+# 'ALTER ... DROP COMPACT STORAGE' is considered experimental and is not recommended for production use.
+enable_drop_compact_storage: false
+
 # The default Windows kernel timer and scheduling resolution is 15.6ms for power conservation.
 # Lowering this value on Windows can provide much tighter latency and better throughput, however
 # some virtualized environments may see a negative performance impact from changing this setting

src/java/org/apache/cassandra/config/Config.java

@@ -322,6 +322,8 @@ public class Config
 
     public boolean enable_materialized_views = true;
 
+    public volatile boolean enable_drop_compact_storage = true;
+
     /**
      * Optionally disable asynchronous UDF execution.
      * Disabling asynchronous UDF execution also implicitly disables the security-manager!

src/java/org/apache/cassandra/config/DatabaseDescriptor.java

@@ -2169,6 +2169,17 @@ public class DatabaseDescriptor
         return conf.enable_materialized_views;
     }
 
+    public static boolean enableDropCompactStorage()
+    {
+        return conf.enable_drop_compact_storage;
+    }
+
+    @VisibleForTesting
+    public static void setEnableDropCompactStorage(boolean enableDropCompactStorage)
+    {
+        conf.enable_drop_compact_storage = enableDropCompactStorage;
+    }
+
     public static long getUserDefinedFunctionFailTimeout()
     {
         return conf.user_defined_function_fail_timeout;

src/java/org/apache/cassandra/cql3/statements/AlterTableStatement.java

@@ -290,6 +290,10 @@ public class AlterTableStatement extends SchemaAlteringStatement
                                                                     columnFamily()));
                 break;
             case DROP_COMPACT_STORAGE:
+
+                if (!DatabaseDescriptor.enableDropCompactStorage())
+                    throw new InvalidRequestException("DROP COMPACT STORAGE is disabled. Enable in cassandra.yaml to use.");
+
                 if (!meta.isCompactTable())
                     throw new InvalidRequestException("Cannot DROP COMPACT STORAGE on table without COMPACT STORAGE");
 

test/conf/cassandra.yaml

@@ -41,3 +41,4 @@ row_cache_class_name: org.apache.cassandra.cache.OHCProvider
 row_cache_size_in_mb: 16
 enable_user_defined_functions: true
 enable_scripted_user_defined_functions: true
+enable_drop_compact_storage: true

test/distributed/org/apache/cassandra/distributed/upgrade/CompactStorage2to3UpgradeTest.java

@@ -165,6 +165,10 @@ public class CompactStorage2to3UpgradeTest extends UpgradeTestBase
 
                     });
 
+                }).runBeforeNodeRestart((cluster, node) ->
+                {
+                    cluster.get(node).config().set("enable_drop_compact_storage", true);
+
                 }).runAfterClusterUpgrade(cluster ->
                 {
                     for (int i = 1; i <= cluster.size(); i++)
@@ -207,7 +211,7 @@ public class CompactStorage2to3UpgradeTest extends UpgradeTestBase
         new TestCase()
                 .nodes(2)
                 .upgrade(Versions.Major.v22, Versions.Major.v30)
-                .withConfig(config -> config.with(GOSSIP, NETWORK, NATIVE_PROTOCOL))
+                .withConfig(config -> config.with(GOSSIP, NETWORK, NATIVE_PROTOCOL).set("enable_drop_compact_storage", true))
                 .setup(cluster -> {
                     cluster.schemaChange(String.format(
                             "CREATE TABLE %s.%s (key int, c1 int, c2 int, c3 int, PRIMARY KEY (key, c1, c2)) WITH COMPACT STORAGE",

test/distributed/org/apache/cassandra/distributed/upgrade/DropCompactStorageTest.java

@@ -48,7 +48,7 @@ public class DropCompactStorageTest extends UpgradeTestBase
         new TestCase()
         .nodes(1)
         .upgrade(Versions.Major.v22, upgradeTo)
-        .withConfig(config -> config.with(GOSSIP, NETWORK, NATIVE_PROTOCOL))
+        .withConfig(config -> config.with(GOSSIP, NETWORK, NATIVE_PROTOCOL).set("enable_drop_compact_storage", true))
         .setup((cluster) -> {
             cluster.schemaChange("CREATE TABLE " + KEYSPACE + ".tbl (id int, ck int, v int, PRIMARY KEY (id, ck)) WITH COMPACT STORAGE");
             for (int i = 0; i < 5; i++)

test/distributed/org/apache/cassandra/distributed/upgrade/UpgradeTestBase.java

@@ -31,7 +31,6 @@ import org.junit.BeforeClass;
 import org.apache.cassandra.distributed.UpgradeableCluster;
 import org.apache.cassandra.distributed.api.ICluster;
 import org.apache.cassandra.distributed.api.IInstanceConfig;
-import org.apache.cassandra.distributed.api.IUpgradeableInstance;
 import org.apache.cassandra.distributed.impl.Instance;
 import org.apache.cassandra.distributed.shared.DistributedTestBase;
 import org.apache.cassandra.distributed.shared.Versions;
@@ -89,6 +88,7 @@ public class UpgradeTestBase extends DistributedTestBase
         private final List<TestVersions> upgrade = new ArrayList<>();
         private int nodeCount = 3;
         private RunOnCluster setup;
+        private RunOnClusterAndNode runBeforeNodeRestart;
         private RunOnClusterAndNode runAfterNodeUpgrade;
         private RunOnCluster runAfterClusterUpgrade;
         private final Set<Integer> nodesToUpgrade = new HashSet<>();
@@ -131,6 +131,12 @@ public class UpgradeTestBase extends DistributedTestBase
             return this;
         }
 
+        public TestCase runBeforeNodeRestart(RunOnClusterAndNode runBeforeNodeRestart)
+        {
+            this.runBeforeNodeRestart = runBeforeNodeRestart;
+            return this;
+        }
+
         public TestCase runAfterNodeUpgrade(RunOnClusterAndNode runAfterNodeUpgrade)
         {
             this.runAfterNodeUpgrade = runAfterNodeUpgrade;
@@ -157,6 +163,8 @@ public class UpgradeTestBase extends DistributedTestBase
                 throw new AssertionError();
             if (runAfterClusterUpgrade == null && runAfterNodeUpgrade == null)
                 throw new AssertionError();
+            if (runBeforeNodeRestart == null)
+                runBeforeNodeRestart = (c, n) -> {};
             if (runAfterClusterUpgrade == null)
                 runAfterClusterUpgrade = (c) -> {};
             if (runAfterNodeUpgrade == null)
@@ -177,6 +185,7 @@ public class UpgradeTestBase extends DistributedTestBase
                         {
                             cluster.get(n).shutdown().get();
                             cluster.get(n).setVersion(version);
+                            runBeforeNodeRestart.run(cluster, n);
                             cluster.get(n).startup();
                             runAfterNodeUpgrade.run(cluster, n);
                         }

test/unit/org/apache/cassandra/cql3/validation/operations/AlterTest.java

@@ -20,6 +20,7 @@ package org.apache.cassandra.cql3.validation.operations;
 import org.junit.Assert;
 import org.junit.Test;
 
+import org.apache.cassandra.config.DatabaseDescriptor;
 import org.apache.cassandra.cql3.CQLTester;
 import org.apache.cassandra.db.ColumnFamilyStore;
 import org.apache.cassandra.db.Keyspace;
@@ -547,4 +548,14 @@ public class AlterTest extends CQLTester
         assertInvalidMessage(table2, format("ALTER TYPE %s.%s ADD v2 int;", keyspace(), type1));
         assertInvalidMessage(table3, format("ALTER TYPE %s.%s ADD v2 int;", keyspace(), type1));
     }
+
+    @Test
+    public void testAlterDropCompactStorageDisabled() throws Throwable
+    {
+        DatabaseDescriptor.setEnableDropCompactStorage(false);
+
+        createTable("CREATE TABLE %s (k text, i int, PRIMARY KEY (k, i)) WITH COMPACT STORAGE");
+
+        assertInvalidMessage("DROP COMPACT STORAGE is disabled. Enable in cassandra.yaml to use.", "ALTER TABLE %s DROP COMPACT STORAGE");
+    }
 }