[GitHub] [cassandra] adelapena commented on a change in pull request #891: CASSANDRA-16415: Fix digest mismatches during upgrades (cassandra-3.11)

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

adelapena commented on a change in pull request #891:
URL: https://github.com/apache/cassandra/pull/891#discussion_r578665271



##########
File path: test/distributed/org/apache/cassandra/distributed/upgrade/MixedModeReadTest.java
##########
@@ -56,12 +44,13 @@ public void mixedModeReadColumnSubsetDigestCheck() throws Throwable
         .withConfig(config -> config.with(Feature.GOSSIP, Feature.NETWORK))
         .setup(cluster -> {
             cluster.schemaChange(CREATE_TABLE);
-            cluster.coordinator(1).execute(INSERT, ConsistencyLevel.ALL, 1, "foo", "bar", "baz");
-            cluster.coordinator(1).execute(INSERT, ConsistencyLevel.ALL, 2, "foo", "bar", "baz");
+            cluster.coordinator(1).execute(INSERT, ConsistencyLevel.ALL, 1, "static", "foo", "bar", "baz");
+            cluster.coordinator(1).execute(INSERT, ConsistencyLevel.ALL, 1, "static", "fi", "biz", "baz");
+            cluster.coordinator(1).execute(INSERT, ConsistencyLevel.ALL, 1, "static", "fo", "boz", "baz");
 
             // baseline to show no digest mismatches before upgrade
-            checkTraceForDigestMismatch(cluster, 1);
-            checkTraceForDigestMismatch(cluster, 2);
+            checkTraceForDigestMismatch(cluster, 1, SELECT_C1, 1);
+            checkTraceForDigestMismatch(cluster, 2, SELECT_C1, 1);
         })
         .runAfterNodeUpgrade((cluster, node) -> {

Review comment:
       I think we could just use `runAfterClusterUpgrade` instead of `runAfterNodeUpgrade` and get rid of the `if (node != 1)` part.

##########
File path: src/java/org/apache/cassandra/db/filter/ColumnFilter.java
##########
@@ -385,44 +404,24 @@ public boolean equals(Object other)
     @Override
     public String toString()
     {
-        if (isFetchAll)
-            return "*";
-
-        if (queried.isEmpty())
-            return "";
-
-        Iterator<ColumnDefinition> defs = queried.selectOrderIterator();
-        if (!defs.hasNext())
-            return "<none>";
-
-        StringBuilder sb = new StringBuilder();
-        while (defs.hasNext())
-        {
-            appendColumnDef(sb, defs.next());
-            if (defs.hasNext())
-                sb.append(", ");
+        if (isFetchAll && queried == null)
+            return "*/*";

Review comment:
       I also like the idea of making it more descriptive, showing which part is the queried one and which is the fetched one, maybe something like `{queried=*, fetched=[v2, v3]}`? I'm not sure whether it would be a too invasive change in the logs, though.

##########
File path: test/distributed/org/apache/cassandra/distributed/upgrade/MixedModeReadTest.java
##########
@@ -18,34 +18,22 @@
 
 package org.apache.cassandra.distributed.upgrade;
 
-import java.util.UUID;
-
-import org.junit.Assert;
 import org.junit.Test;
 
-import org.apache.cassandra.distributed.UpgradeableCluster;
 import org.apache.cassandra.distributed.api.ConsistencyLevel;
 import org.apache.cassandra.distributed.api.Feature;
-import org.apache.cassandra.distributed.impl.DelegatingInvokableInstance;
-import org.apache.cassandra.distributed.shared.DistributedTestBase;
+import org.apache.cassandra.distributed.api.IInvokableInstance;
 import org.apache.cassandra.distributed.shared.Versions;
 import org.apache.cassandra.gms.Gossiper;
 
+import static org.apache.cassandra.distributed.test.ReadDigestConsistencyTest.CREATE_TABLE;
+import static org.apache.cassandra.distributed.test.ReadDigestConsistencyTest.INSERT;
+import static org.apache.cassandra.distributed.test.ReadDigestConsistencyTest.SELECT_C1;
+import static org.apache.cassandra.distributed.test.ReadDigestConsistencyTest.SELECT_C1_S1_ROW;
+import static org.apache.cassandra.distributed.test.ReadDigestConsistencyTest.checkTraceForDigestMismatch;

Review comment:
       Since we are reusing `ReadDigestConsistencyTest` we could add a couple of methods to it for populating the table and do the queries, something like:
   ```java
   public static void initialize(AbstractCluster<?> cluster)
   {
       cluster.schemaChange(CREATE_TABLE);
       cluster.coordinator(1).execute(INSERT, ConsistencyLevel.ALL, 1, "static", "foo", "bar", "baz");
       cluster.coordinator(1).execute(INSERT, ConsistencyLevel.ALL, 1, "static", "fi", "biz", "baz");
       cluster.coordinator(1).execute(INSERT, ConsistencyLevel.ALL, 1, "static", "fo", "boz", "baz");
   }
   
   public static void checkTraceForDigestMismatch(AbstractCluster<?> cluster)
   {
       checkTraceForDigestMismatch(cluster, 1, SELECT_C1, 1);
       checkTraceForDigestMismatch(cluster, 2, SELECT_C1, 1);
       checkTraceForDigestMismatch(cluster, 1, SELECT_C1_S1_ROW, 1, "foo");
       checkTraceForDigestMismatch(cluster, 2, SELECT_C1_S1_ROW, 1, "fi");
   }
   ```
   We would call these two methods from the upgrade test and from `ReadDigestConsistencyTest#testDigestConsistency`, avoiding the duplication of these two blocks of code and reducing the number of imports, wdyt? We could also put the two methods in a shared helper class, I don't have any preference here.




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

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