yifan-c commented on a change in pull request #892:
URL: https://github.com/apache/cassandra/pull/892#discussion_r581300735
##########
File path: src/java/org/apache/cassandra/gms/Gossiper.java
##########
@@ -2128,9 +2143,25 @@ public boolean waitForSchemaAgreement(long maxWait,
TimeUnit unit, BooleanSuppli
}
}
- public boolean haveMajorVersion3Nodes()
+ /**
+ * Returns {@code true} only if the information about the version of each
node in the cluster is available and
+ * ALL the nodes are on 4.0 (regardless of the patch version).
+ */
+ public boolean hasMajorVersion3Nodes()
{
- return haveMajorVersion3NodesMemoized.get();
+
Review comment:
nit: remove the empty line.
##########
File path: src/java/org/apache/cassandra/gms/Gossiper.java
##########
@@ -2128,9 +2143,25 @@ public boolean waitForSchemaAgreement(long maxWait,
TimeUnit unit, BooleanSuppli
}
}
- public boolean haveMajorVersion3Nodes()
+ /**
+ * Returns {@code true} only if the information about the version of each
node in the cluster is available and
+ * ALL the nodes are on 4.0 (regardless of the patch version).
+ */
+ public boolean hasMajorVersion3Nodes()
{
- return haveMajorVersion3NodesMemoized.get();
+
+ return upgradeInProgressPossible ||
isUpgradingFromVersionLowerThan(CassandraVersion.CASSANDRA_4_0);
Review comment:
should the condition be like the below?
```java
upgradeInProgressPossible &&
isUpgradingFromVersionLowerThan(CassandraVersion.CASSANDRA_4_0);
```
The current behavior (using `||`) is we assume the cluster has v30 nodes
when the node does not have version info of all the peers. When
`upgradeInProgressPossible == true`, any minVersion memorized in the supplier
above is just ignored...
If the current behavior is desired, please add comment to the code.
##########
File path: src/java/org/apache/cassandra/gms/Gossiper.java
##########
@@ -158,38 +158,53 @@
private volatile long lastProcessedMessageAt = System.currentTimeMillis();
- //This property and anything that checks it should be removed in 5.0
- private boolean haveMajorVersion3Nodes = true;
+ // This property and anything that checks it should be removed in 5.0
+ /**
+ * This property is initially set to {@code true} which means that we have
no information about the other nodes.
Review comment:
nit: please merge those 2 comments.
```suggestion
/**
* This property and anything that checks it should be removed in 5.0
*
* This property is initially set to {@code true} which means that we
have no information about the other nodes.
```
##########
File path: src/java/org/apache/cassandra/gms/Gossiper.java
##########
@@ -2128,9 +2143,25 @@ public boolean waitForSchemaAgreement(long maxWait,
TimeUnit unit, BooleanSuppli
}
}
- public boolean haveMajorVersion3Nodes()
+ /**
+ * Returns {@code true} only if the information about the version of each
node in the cluster is available and
+ * ALL the nodes are on 4.0 (regardless of the patch version).
Review comment:
The comment does not seem to describe the method correctly. I think you
want the below according to the current behavior.
```suggestion
* Returns {@code false} only if the information about the version of
each node in the cluster is available and
* ALL the nodes are on 4.0 (regardless of the patch version).
```
##########
File path: src/java/org/apache/cassandra/utils/ExpiringMemoizingSupplier.java
##########
@@ -77,6 +77,11 @@ public T get() {
return this.value;
}
+ public void expire()
Review comment:
nit: annotate with `@VisibleForTesting`? It is only used for testing.
##########
File path: src/java/org/apache/cassandra/gms/Gossiper.java
##########
@@ -158,38 +158,53 @@
private volatile long lastProcessedMessageAt = System.currentTimeMillis();
- //This property and anything that checks it should be removed in 5.0
- private boolean haveMajorVersion3Nodes = true;
+ // This property and anything that checks it should be removed in 5.0
+ /**
+ * This property is initially set to {@code true} which means that we have
no information about the other nodes.
+ * Once all nodes are on at least this node version, it becomes {@code
false}, which means that we are not
+ * upgrading from the previous version (major, minor).
+ */
+ private boolean upgradeInProgressPossible = true;
Review comment:
We should add `volatile` to the variable, since it is now read by both
the supplier and `hasMajorVersion3Nodes()`. It can be accessed from multiple
threads.
Before the patch, the variable is only accessed by
`ExpiringMemoizingSupplier`.
----------------------------------------------------------------
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]
