This is an automated email from the ASF dual-hosted git repository.

blambov pushed a commit to branch cassandra-4.1
in repository https://gitbox.apache.org/repos/asf/cassandra.git


The following commit(s) were added to refs/heads/cassandra-4.1 by this push:
     new 9f58d76f38 Avoid schema mismatch problems on memtable API 
misconfiguration
9f58d76f38 is described below

commit 9f58d76f3841864be11f5b9c4534027451328569
Author: Branimir Lambov <branimir.lam...@datastax.com>
AuthorDate: Mon Nov 14 13:59:05 2022 +0200

    Avoid schema mismatch problems on memtable API misconfiguration
    
    patch by Branimir Lambov; reviewed by Caleb Rackliffe for CASSANDRA-18040
---
 .../apache/cassandra/db/memtable/Memtable_API.md   |  7 +-
 .../apache/cassandra/schema/MemtableParams.java    | 18 +++++
 .../apache/cassandra/schema/SchemaKeyspace.java    |  4 +-
 .../cassandra/distributed/test/AlterTest.java      | 82 ++++++++++++++++++++++
 4 files changed, 107 insertions(+), 4 deletions(-)

diff --git a/src/java/org/apache/cassandra/db/memtable/Memtable_API.md 
b/src/java/org/apache/cassandra/db/memtable/Memtable_API.md
index 39f9b201af..8a582e3ad9 100644
--- a/src/java/org/apache/cassandra/db/memtable/Memtable_API.md
+++ b/src/java/org/apache/cassandra/db/memtable/Memtable_API.md
@@ -72,9 +72,10 @@ ALTER TABLE ... WITH memtable = 'default';
 ```
 
 The memtable configuration selection is per table, i.e. it will be propagated 
to all nodes in the cluster. If some nodes
-do not have a definition for that configuration, cannot instantiate the class, 
or are still on a version of Cassandra
-before 4.1, they will reject the schema change. We therefore recommend using a 
separate `ALTER` statement to change a
-table's memtable implementation; upgrading all nodes to 4.1 or later is 
required to use the API.
+do not have a definition for that configuration or cannot instantiate the 
class, they will log an error and fall 
+back to the default memtable configuration to avoid schema disagreements. 
However, if some nodes are still on a version 
+of Cassandra before 4.1, they will reject the schema change. We therefore 
recommend using a separate `ALTER` statement 
+to change a table's memtable implementation; upgrading all nodes to 4.1 or 
later is required to use the API.
 
 As additional safety when first deploying an alternative implementation to a 
production cluster, one may consider
 first deploying a remapped `default` configuration to all nodes in the 
cluster, switching the schema to reference
diff --git a/src/java/org/apache/cassandra/schema/MemtableParams.java 
b/src/java/org/apache/cassandra/schema/MemtableParams.java
index a3f1bb2323..3470b7ac8d 100644
--- a/src/java/org/apache/cassandra/schema/MemtableParams.java
+++ b/src/java/org/apache/cassandra/schema/MemtableParams.java
@@ -28,6 +28,8 @@ import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Objects;
 import com.google.common.collect.ImmutableMap;
 
+import org.slf4j.LoggerFactory;
+
 import org.apache.cassandra.config.DatabaseDescriptor;
 import org.apache.cassandra.config.InheritingClass;
 import org.apache.cassandra.config.ParameterizedClass;
@@ -110,6 +112,22 @@ public final class MemtableParams
         }
     }
 
+    public static MemtableParams getWithFallback(String key)
+    {
+        try
+        {
+            return get(key);
+        }
+        catch (ConfigurationException e)
+        {
+            LoggerFactory.getLogger(MemtableParams.class).error("Invalid 
memtable configuration \"" + key + "\" in schema. " +
+                                                                "Falling back 
to default to avoid schema mismatch.\n" +
+                                                                "Please ensure 
the correct definition is given in cassandra.yaml.",
+                                                                e);
+            return new MemtableParams(DEFAULT.factory(), key);
+        }
+    }
+
     @VisibleForTesting
     static Map<String, ParameterizedClass> expandDefinitions(Map<String, 
InheritingClass> memtableConfigurations)
     {
diff --git a/src/java/org/apache/cassandra/schema/SchemaKeyspace.java 
b/src/java/org/apache/cassandra/schema/SchemaKeyspace.java
index 33d2b7d6c8..b4f27830ef 100644
--- a/src/java/org/apache/cassandra/schema/SchemaKeyspace.java
+++ b/src/java/org/apache/cassandra/schema/SchemaKeyspace.java
@@ -960,7 +960,9 @@ public final class SchemaKeyspace
                           .comment(row.getString("comment"))
                           
.compaction(CompactionParams.fromMap(row.getFrozenTextMap("compaction")))
                           
.compression(CompressionParams.fromMap(row.getFrozenTextMap("compression")))
-                          .memtable(MemtableParams.get(row.has("memtable") ? 
row.getString("memtable") : null)) // memtable column was introduced in 4.1
+                          
.memtable(MemtableParams.getWithFallback(row.has("memtable")
+                                                                   ? 
row.getString("memtable")
+                                                                   : null)) // 
memtable column was introduced in 4.1
                           
.defaultTimeToLive(row.getInt("default_time_to_live"))
                           .extensions(row.getFrozenMap("extensions", 
UTF8Type.instance, BytesType.instance))
                           .gcGraceSeconds(row.getInt("gc_grace_seconds"))
diff --git 
a/test/distributed/org/apache/cassandra/distributed/test/AlterTest.java 
b/test/distributed/org/apache/cassandra/distributed/test/AlterTest.java
index b8912b262f..2061c29141 100644
--- a/test/distributed/org/apache/cassandra/distributed/test/AlterTest.java
+++ b/test/distributed/org/apache/cassandra/distributed/test/AlterTest.java
@@ -18,17 +18,29 @@
 
 package org.apache.cassandra.distributed.test;
 
+import java.util.List;
+
+import com.google.common.collect.ImmutableMap;
 import org.junit.Assert;
 import org.junit.Test;
 
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import org.apache.cassandra.cql3.Lists;
 import org.apache.cassandra.db.Keyspace;
 import org.apache.cassandra.distributed.Cluster;
+import org.apache.cassandra.distributed.api.Feature;
 import org.apache.cassandra.distributed.api.ICluster;
 import org.apache.cassandra.distributed.api.IInstanceConfig;
 import org.apache.cassandra.distributed.api.IInvokableInstance;
 import org.apache.cassandra.distributed.api.IIsolatedExecutor;
 import org.apache.cassandra.distributed.api.SimpleQueryResult;
+import org.apache.cassandra.distributed.api.TokenSupplier;
+import org.apache.cassandra.distributed.shared.ClusterUtils;
+import org.apache.cassandra.distributed.util.QueryResultUtil;
 import org.apache.cassandra.service.StorageService;
+import org.apache.cassandra.utils.Throwables;
 
 import static 
org.apache.cassandra.distributed.action.GossipHelper.withProperty;
 import static org.apache.cassandra.distributed.api.ConsistencyLevel.ONE;
@@ -39,6 +51,7 @@ import static 
org.apache.cassandra.distributed.api.TokenSupplier.evenlyDistribut
 import static 
org.apache.cassandra.distributed.shared.NetworkTopology.singleDcNetworkTopology;
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.fail;
 
 public class AlterTest extends TestBaseImpl
 {
@@ -107,4 +120,73 @@ public class AlterTest extends TestBaseImpl
             }
         }
     }
+
+    @Test
+    public void unknownMemtableConfigurationTest() throws Throwable
+    {
+        Logger logger = LoggerFactory.getLogger(getClass());
+        try (Cluster cluster = Cluster.build(1)
+                                      
.withTokenSupplier(TokenSupplier.evenlyDistributedTokens(3, 1))
+                                      .withConfig(c -> c.with(Feature.values())
+                                                        .set("memtable", 
ImmutableMap.of(
+                                                        "configurations", 
ImmutableMap.of(
+                                                            "testconfig", 
ImmutableMap.of(
+                                                                "class_name", 
"SkipListMemtable")))))
+                                      .start())
+        {
+            init(cluster);
+            cluster.schemaChange("CREATE TABLE " + KEYSPACE + ".tbl (pk int 
PRIMARY KEY)");
+
+            // Start Node 2 without the memtable configuration definition.
+            IInvokableInstance node1 = cluster.get(1);
+            IInvokableInstance node2 = ClusterUtils.addInstance(cluster, 
node1.config(), c -> c.set("memtable", ImmutableMap.of()));
+            node2.startup(cluster);
+
+            try
+            {
+                cluster.schemaChange("ALTER TABLE " + KEYSPACE + ".tbl WITH 
memtable = 'testconfig'", false, node2);
+                fail("Expected ALTER to fail with unknown memtable 
configuration.");
+            }
+            catch (Throwable t)
+            {
+                // expected
+                logger.info("Expected: {}", t.getMessage());
+                Assert.assertTrue(Throwables.isCausedBy(t, x -> 
x.getMessage().matches("Memtable configuration.*not found.*")));
+            }
+            long mark = node2.logs().mark();
+
+            cluster.schemaChange("ALTER TABLE " + KEYSPACE + ".tbl WITH 
memtable = 'testconfig'", false, node1);
+            // the above should succeed, the configuration is acceptable to 
node1
+
+            final String schema1 = 
QueryResultUtil.expand(node1.executeInternalWithResult("SELECT * FROM 
system_schema.tables WHERE keyspace_name=?", KEYSPACE));
+            final String schema2 = 
QueryResultUtil.expand(node2.executeInternalWithResult("SELECT * FROM 
system_schema.tables WHERE keyspace_name=?", KEYSPACE));
+            logger.info("node1 schema: \n{}", schema1);
+            logger.info("node2 schema: \n{}", schema2);
+            Assert.assertEquals(schema1, schema2);
+            List<String> errorInLog = node2.logs().grep(mark, "ERROR.*Invalid 
memtable configuration.*").getResult();
+            Assert.assertTrue(errorInLog.size() > 0);
+            logger.info(Lists.listToString(errorInLog));
+
+            // Add a new node that has an invalid definition but should accept 
the already defined table schema.
+            IInvokableInstance node3 = ClusterUtils.addInstance(cluster,
+                                                                node2.config(),
+                                                                c -> 
c.set("memtable", ImmutableMap.of(
+                                                                
"configurations", ImmutableMap.of(
+                                                                    
"testconfig", ImmutableMap.of(
+                                                                        
"class_name", "NotExistingMemtable")))));
+            node3.startup(cluster);
+            final String schema3 = 
QueryResultUtil.expand(node3.executeInternalWithResult("SELECT * FROM 
system_schema.tables WHERE keyspace_name=?", KEYSPACE));
+            logger.info("node3 schema: \n{}", schema3);
+            Assert.assertEquals(schema1, schema3);
+
+            errorInLog = node3.logs().grep("ERROR.*Invalid memtable 
configuration.*").getResult();
+            Assert.assertTrue(errorInLog.size() > 0);
+            logger.info(Lists.listToString(errorInLog));
+
+            // verify that all nodes can write to the table
+            node1.executeInternalWithResult("INSERT INTO " + KEYSPACE + ".tbl 
(pk) VALUES (?)", 1);
+            node2.executeInternalWithResult("INSERT INTO " + KEYSPACE + ".tbl 
(pk) VALUES (?)", 2);
+            node3.executeInternalWithResult("INSERT INTO " + KEYSPACE + ".tbl 
(pk) VALUES (?)", 3);
+        }
+    }
 }


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org
For additional commands, e-mail: commits-h...@cassandra.apache.org

Reply via email to