yifan-c commented on a change in pull request #891:
URL: https://github.com/apache/cassandra/pull/891#discussion_r575737797
##########
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:
Since you are changing the toString method already, maybe make it a bit
more descriptive? Is the first `*` for fetched and the second for `queried`?
##########
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:
Is there any code or test depends on the column filter toString output?
##########
File path: src/java/org/apache/cassandra/db/filter/ColumnFilter.java
##########
@@ -505,9 +504,16 @@ public ColumnFilter deserialize(DataInputPlus in, int
version, CFMetaData metada
}
}
- // See CASSANDRA-15833
- if (version <= MessagingService.VERSION_3014 && isFetchAll)
+ // Since nodes with and without CASSANDRA-10657 are not
distinguishable by messaging version, we need to
+ // check whether there are any nodes running pre CASSANDRA-10657
Cassandra and apply conversion to the
+ // column filter so that it is interpreted in the same way as on
the nodes without CASSANDRA-10657.
+ //
+ // see CASSANDRA-10657, CASSANDRA-15833, CASSANDRA-16415
+ if (isFetchAll && queried != null &&
Gossiper.instance.isAnyNodeOn30())
Review comment:
Thanks for the comment.
The previous condition was wrong. Cassandra 3.11 uses
`MessagingService.VERSION_3014`. Checking `version <=
MessagingService.VERSION_3014` incorrectly nullifies `queried`.
The new condition looks good.
However, my concern is that gossip is not very reliable. (Not saying the fix
is bad. It is better than the previous one) Checking
`Gossiper.instance.isAnyNodeOn30()` might still leave a small time window of
digest mismatch during upgrade. The mixed mode scenario happens during
upgrading a cluster. We can assume the value of `has30Node` on each instance
only changes in 1 direction, i.e. true --> false. It will never be the other
way around. However, at a given time point, the status of the ring can be
inconsistent among the nodes due to the gossip mechanism. Some nodes can learn
that all nodes are now in 3.11 (which is the fact), but the others disagree due
to the delay. In such scenario, if the coordinator and replica 1 (R1) thinks
all nodes are already in 3.11, but replica 2 (R2) thinks that there are nodes
still in 3.0, the digest returned from R1 mismatches the one from R2.
I think a small time window is tolerable. Read latencies can go up. But
eventually, when the gossip status is propagated to the entire cluster, the
mismatch should disappear. WDYT?
##########
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 "*/*";
+
+ if (isFetchAll && queried.isEmpty())
+ return "*/[]";
+
+ StringJoiner joiner = new StringJoiner(", ", isFetchAll ? "*/[" : "[",
"]");
+ Iterator<ColumnDefinition> it = queried.selectOrderIterator();
+ while (it.hasNext()) {
Review comment:
move `{` to the new line.
----------------------------------------------------------------
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]
