yifan-c commented on a change in pull request #892:
URL: https://github.com/apache/cassandra/pull/892#discussion_r576534635
##########
File path:
test/distributed/org/apache/cassandra/distributed/upgrade/MixedModeReadTest.java
##########
@@ -67,26 +60,24 @@ public void mixedModeReadColumnSubsetDigestCheck() throws
Throwable
if (node != 1)
return; // shouldn't happen but guard for future test changes
+
+ // we need to let gossip settle or the test will fail
+ int attempts = 1;
+ //noinspection Convert2MethodRef
+ while (!((IInvokableInstance) (cluster.get(1))).callOnInstance(()
-> Gossiper.instance.haveMajorVersion3Nodes()))
+ {
+ if (attempts++ > 30)
+ throw new
RuntimeException("Gossiper.instance.isAnyNodeOn30() continually returns false
despite expecting to be true");
+ Thread.sleep(1000);
+ }
+
// should not cause a disgest mismatch in mixed mode
+ logger.info("Testing queries after upgrading - node1: {}, node2:
{}", cluster.get(1).getReleaseVersionString(),
cluster.get(2).getReleaseVersionString());
Review comment:
How about asserting the release version strings of the nodes instead of
logging?
##########
File path: src/java/org/apache/cassandra/gms/Gossiper.java
##########
@@ -158,38 +158,44 @@
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 null version which means that we have
no information about the other nodes.
+ * Once we have information about the cluster, it is set to the earliest
version encountered in the cluster.
+ * Once all nodes are on at least this node version, it becomes null,
which means that we are not upgrading from
+ * the previous version (major, minor).
+ */
+ private CassandraVersion upgradeFromVersion = SystemKeyspace.NULL_VERSION;
- final Supplier<ExpiringMemoizingSupplier.ReturnValue<Boolean>>
haveMajorVersion3NodesSupplier = () ->
+ final Supplier<ExpiringMemoizingSupplier.ReturnValue<CassandraVersion>>
upgradeFromVersionSupplier = () ->
{
- //Once there are no prior version nodes we don't need to keep
rechecking
- if (!haveMajorVersion3Nodes)
- return new ExpiringMemoizingSupplier.Memoized<>(false);
+ // Once there are no prior version nodes we don't need to keep
rechecking
+ if (upgradeFromVersion == null)
+ return new ExpiringMemoizingSupplier.Memoized<>(null);
Iterable<InetAddressAndPort> allHosts =
Iterables.concat(Gossiper.instance.getLiveMembers(),
Gossiper.instance.getUnreachableMembers());
- CassandraVersion referenceVersion = null;
+ CassandraVersion minVersion = SystemKeyspace.CURRENT_VERSION;
for (InetAddressAndPort host : allHosts)
{
CassandraVersion version = getReleaseVersion(host);
//Raced with changes to gossip state, wait until next iteration
if (version == null)
- return new ExpiringMemoizingSupplier.NotMemoized(true);
-
- if (referenceVersion == null)
- referenceVersion = version;
+ return new
ExpiringMemoizingSupplier.NotMemoized<>(SystemKeyspace.NULL_VERSION);
- if (version.major < 4)
- return new ExpiringMemoizingSupplier.Memoized<>(true);
+ if (version.isLowerThan(minVersion.major, minVersion.minor)) {
Review comment:
```suggestion
if (version.compareTo(minVersion) < 0) {
```
##########
File path: src/java/org/apache/cassandra/utils/CassandraVersion.java
##########
@@ -200,6 +200,11 @@ private static Integer tryParseInt(String str)
}
}
+ public boolean isLowerThan(int major, int minor)
Review comment:
nit: the method can be removed. The "lower than" relationship can be
expressed using compareTo, which is more comprehensive.
```java
// is version lower than 4.0.0?
version.compareTo(new CassandraVersion("4.0.0")) < 0
```
Having multiple methods that compare versions could be confusing.
##########
File path: src/java/org/apache/cassandra/gms/Gossiper.java
##########
@@ -2130,7 +2136,12 @@ public boolean waitForSchemaAgreement(long maxWait,
TimeUnit unit, BooleanSuppli
public boolean haveMajorVersion3Nodes()
{
- return haveMajorVersion3NodesMemoized.get();
+ return isUpgradingFromVersionLowerThan(4, 0);
+ }
+
+ public boolean isUpgradingFromVersionLowerThan(int major, int minor) {
+ CassandraVersion v = upgradeFromVersionMemoized.get();
+ return v != null && v.isLowerThan(major, minor);
Review comment:
`upgradeFromVersionMemoized` can also return `NULL_VERSION`, which is
lower than any valid version... This is probably not the desired output.
----------------------------------------------------------------
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]
