adelapena commented on a change in pull request #892:
URL: https://github.com/apache/cassandra/pull/892#discussion_r579295720
##########
File path: test/unit/org/apache/cassandra/db/filter/ColumnFilterTest.java
##########
@@ -60,17 +91,13 @@ public void testColumnFilterSerialisationRoundTrip() throws
Exception
ColumnFilter columnFilter;
columnFilter = ColumnFilter.all(metadata);
- testRoundTrip(columnFilter,
ColumnFilter.Serializer.maybeUpdateForBackwardCompatility(columnFilter,
MessagingService.VERSION_30), metadata, MessagingService.VERSION_30);
Review comment:
Now only one of the versions of `testRoundTrip` is called by the tests,
we could have a single version of the method.
##########
File path: src/java/org/apache/cassandra/db/SystemKeyspace.java
##########
@@ -90,6 +90,8 @@ private SystemKeyspace()
// Cassandra was not previously installed and we're in the process of
starting a fresh node.
public static final CassandraVersion NULL_VERSION = new
CassandraVersion("0.0.0-absent");
+ public static final CassandraVersion CURRENT_VERSION = new
CassandraVersion(FBUtilities.getReleaseVersionString());
Review comment:
There is a usage of `new
CassandraVersion(FBUtilities.getReleaseVersionString())` below in
`SystemKeyspace.getReleaseVersion`, we could use there the new
`CURRENT_VERSION` constant.
##########
File path: src/java/org/apache/cassandra/db/filter/ColumnFilter.java
##########
@@ -74,25 +78,34 @@
// null. If false, then _fetched_ == _queried_ and we only store _queried_.
final boolean fetchAllRegulars;
+ // This flag can be only set when fetchAllRegulars is set. When
fetchAllRegulars is set and queried==null then
+ // it is implied to be true. The flag when set allows for interpreting the
column filter in the same way as it was
+ // interpreted by pre 4.0 Cassandra versions (3.4 ~ 4.0), that is, we
fetch all columns (both regulars and static)
+ // but we query only some of them. This allows for proper behaviour during
upgrades.
+ final boolean fetchAllStatics;
Review comment:
We could verify this value in
`ColumnFilterTest#testColumnFilterConstruction`, where we verify the value of
`fetchAllRegulars`. By the way, I think that all the attributes with default
visibility here could use `@VisibleForTesting`.
##########
File path: test/unit/org/apache/cassandra/db/filter/ColumnFilterTest.java
##########
@@ -18,31 +18,62 @@
package org.apache.cassandra.db.filter;
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertFalse;
-import static org.junit.Assert.assertNull;
-import static org.junit.Assert.assertTrue;
+import java.util.Arrays;
+import java.util.Collection;
+
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.BeforeClass;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+import org.apache.cassandra.Util;
import org.apache.cassandra.db.RegularAndStaticColumns;
import org.apache.cassandra.db.marshal.Int32Type;
import org.apache.cassandra.db.marshal.SetType;
import org.apache.cassandra.db.rows.CellPath;
import org.apache.cassandra.dht.Murmur3Partitioner;
+import org.apache.cassandra.gms.Gossiper;
import org.apache.cassandra.io.util.DataInputBuffer;
import org.apache.cassandra.io.util.DataInputPlus;
import org.apache.cassandra.io.util.DataOutputBuffer;
import org.apache.cassandra.net.MessagingService;
import org.apache.cassandra.schema.ColumnMetadata;
import org.apache.cassandra.schema.TableMetadata;
import org.apache.cassandra.utils.ByteBufferUtil;
-import org.junit.Test;
-import org.junit.Assert;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertTrue;
+@RunWith(Parameterized.class)
public class ColumnFilterTest
{
private static final ColumnFilter.Serializer serializer = new
ColumnFilter.Serializer();
+ @Parameterized.Parameter(0)
+ public String clusterMinVersion;
Review comment:
I like the parameterized approach 👍
##########
File path: test/unit/org/apache/cassandra/Util.java
##########
@@ -797,4 +797,16 @@ public static void disableBloomFilter(ColumnFamilyStore
cfs)
for (SSTableReader reader : cfs.getLiveSSTables())
assertEquals(FilterFactory.AlwaysPresent, reader.getBloomFilter());
}
+
+ public static void setUpgradeFromVersion(String version)
Review comment:
Could we add some JavaDoc here?
##########
File path: test/unit/org/apache/cassandra/db/filter/ColumnFilterTest.java
##########
@@ -208,6 +241,38 @@ private static void testRoundTrip(ColumnFilter
columnFilter, ColumnFilter expect
Assert.assertEquals(deserialized, expected);
}
+ @Test
+ public void testToString()
+ {
+ TableMetadata metadata = TableMetadata.builder("ks", "table")
Review comment:
As [suggested in the PR for
3.0](https://github.com/apache/cassandra/pull/890#discussion_r578410628), we
could include a multi-cell column to the table metadata to test how
subselections are printed.
##########
File path: src/java/org/apache/cassandra/db/filter/ColumnFilter.java
##########
@@ -271,15 +329,15 @@ public Tester newTester(ColumnMetadata column)
if (s.isEmpty())
return null;
- return new Tester(!column.isStatic() && fetchAllRegulars,
s.iterator());
+ return new Tester(!column.isStatic() && fetchAllRegulars ||
column.isStatic() && fetchAllStatics, s.iterator());
}
/**
* Given an iterator on the cell of a complex column, returns an iterator
that only include the cells selected by
* this filter.
*
* @param column the (complex) column for which the cells are.
- * @param cells the cells to filter.
+ * @param cells the cells to filter.
Review comment:
```suggestion
* @param cells the cells to filter.
```
##########
File path: src/java/org/apache/cassandra/gms/Gossiper.java
##########
@@ -2130,7 +2148,12 @@ public boolean waitForSchemaAgreement(long maxWait,
TimeUnit unit, BooleanSuppli
public boolean haveMajorVersion3Nodes()
Review comment:
I'm not a native English speaker but, shouldn't this be
`hasMajorVersion3Nodes`?
##########
File path: src/java/org/apache/cassandra/db/filter/ColumnFilter.java
##########
@@ -483,84 +549,56 @@ public boolean equals(Object other)
@Override
public String toString()
{
- if (fetchAllRegulars && queried == null)
- return "*";
+ String prefix = "";
- if (queried.isEmpty())
- return "";
+ if (fetchAllRegulars && queried == null)
+ return "*/*";
- Iterator<ColumnMetadata> defs = queried.selectOrderIterator();
- if (!defs.hasNext())
- return "<none>";
+ if (fetchAllRegulars && fetchAllStatics)
+ prefix = "*/";
- StringBuilder sb = new StringBuilder();
- while (defs.hasNext())
+ if (fetchAllRegulars && !fetchAllStatics)
{
- appendColumnDef(sb, defs.next());
- if (defs.hasNext())
- sb.append(", ");
+ prefix = queried.statics.isEmpty()
+ ? "<all regulars>/"
+ : String.format("<all regulars>+%s/",
columnsToString(queried.statics::selectOrderIterator));
Review comment:
I wonder if it would be easier to understand if we simply print the
specific fetched columns, so the only abbreviation used by `toString` is `*`.
----------------------------------------------------------------
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]
