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:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
