onichols-pivotal commented on a change in pull request #5462:
URL: https://github.com/apache/geode/pull/5462#discussion_r472588532
##########
File path:
geode-gfsh/src/main/java/org/apache/geode/management/internal/cli/commands/ConnectCommand.java
##########
@@ -186,6 +186,16 @@ public ResultModel connect(
gfsh.logInfo("failed to get the the remote version.", ex);
}
+ // fallback: see if serialization versions matches (Geode 1.12 or later
cluster)
+ try {
+ String gfshGeodeSerializationVersion =
gfsh.getGeodeSerializationVersion();
+ String remoteGeodeSerializationVersion =
invoker.getRemoteGeodeSerializationVersion();
+ if
(gfshGeodeSerializationVersion.equals(remoteGeodeSerializationVersion)) {
+ return result;
+ }
+ } catch (Exception ignore) {
+ }
+
Review comment:
I've moved the logic for determining compatible clusters into a central
method to decouple from the network calls needed to get the necessary info.
It's a lot easier now to test just this method in isolation.
Rather than add even further assumptions about product name (even if it were
available from the remote cluster) I've constrained the logic that uses product
version to the narrowest possible scenario (specific known versions and only
used if the remote cluster does not report serialization version)
##########
File path:
geode-gfsh/src/test/java/org/apache/geode/management/internal/cli/commands/ConnectCommandTest.java
##########
@@ -417,4 +417,21 @@ public void connectToManagerBefore1_10() {
.statusIsError()
.containsOutput("Cannot use a 1.14 gfsh client to connect to a 1.9
cluster");
}
+
+ @Test
+ public void connectToManagerBySerializationVersion() {
+ when(gfsh.getVersion()).thenReturn("0.0.0");
+ when(gfsh.getGeodeSerializationVersion()).thenReturn("1.14.0");
+ when(operationInvoker.getRemoteVersion()).thenReturn("0.0.0");
+
when(operationInvoker.getRemoteGeodeSerializationVersion()).thenReturn("1.14.0");
+ when(operationInvoker.isConnected()).thenReturn(true);
+
+ ResultModel resultModel = new ResultModel();
+ when(connectCommand.jmxConnect(any(), anyBoolean(), any(), any(),
anyBoolean()))
+ .thenReturn(resultModel);
+
+ gfshParserRule.executeAndAssertThat(connectCommand, "connect
--locator=localhost:4040")
+ .statusIsSuccess()
+ .doesNotContainOutput("Cannot use a 0.0.0 gfsh client to connect to a
0.0.0 cluster");
+ }
Review comment:
The different-minor-version test was a holdover from the old days when
gfsh was locked to same major&minor as the cluster. I removed it. I think
major versions still play a role, as we need to carefully consider how far into
the future we are willing to guarantee forward compatibility. It seems safest
for now to presume that gfsh 1.x won't be interoperable with a future Geode
2.x. The reverse might be more plausible but we can unlock backward major
version compatibility if and when the time comes.
----------------------------------------------------------------
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:
us...@infra.apache.org
