[
https://issues.apache.org/jira/browse/GEODE-8435?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17179966#comment-17179966
]
ASF GitHub Bot commented on GEODE-8435:
---------------------------------------
Bill commented on a change in pull request #5462:
URL: https://github.com/apache/geode/pull/5462#discussion_r472360083
##########
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:
This is my understanding of the equivalence classes for testing are (rsv
= remote serialization version read via JMX, rmv = remote marketing version
read via JMX)
```
rsv >= 1.12 && rmv == dont-care
compatible, covered by connectToManagerBySerializationVersion()
rsv < 1.12
impossible
rsv unknown
rmv >= 1.10
compatible, covered by connectToManagerOlderThan1_10() sic
rmv < 1.10
incompatible
rmv unknown
incompatible
```
By the way `connectToManagerOlderThan1_10()` is mis-named: it's actually
connecting to version 1.10—not a version _older_ than 1.10. Please fix this
name.
But then I also see these tests:
```
connectToManagerWithDifferentMajorVersion()
connectToManagerWithDifferentMinorVersion()
```
So major and minor versions seem to be important too. But only for certain
version ranges? Actually `ConnectCommand` does not, in general respect semver
at all. I think those tests need to be removed.
##########
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:
Intuitively, it seems that this logic should be tried first and that the
alternative (examining the marketing version) should be the fallback.
Another issue with this block of code is that the exception is silently
ignored. At a minimum we need a clear comment explaining where the exception
comes from since none of the method signatures inside the block have `throws`
clauses. Seems like we could eliminate this `try-catch` entirely.
Also, it would aid in maintenance if the code on lines 171-197 was extracted
into a method. This would simplify `ConnectCommand.connect()` and would give us
the opportunity to name the new method, hide variables, etc. By the way, the
comment at line 171 lies:
```
// since 1.14, only allow gfsh to connect to cluster that's older than
1.10
```
Should say "newer" not "older".
Um also, the logic on line 178-179:
```
if (versionComponent(remoteVersion, VERSION_MAJOR).equals("1") &&
minorVersion >= 10 ||
versionComponent(remoteVersion, VERSION_MAJOR).equals("9") &&
minorVersion >= 9) {
```
Needs a gating condition that checks the product name so that these version
checks only succeed for Geode and GemFire respectively.
----------------------------------------------------------------
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]
> restore gfsh ability to connect if serialization versions match regardless of
> product version
> ---------------------------------------------------------------------------------------------
>
> Key: GEODE-8435
> URL: https://issues.apache.org/jira/browse/GEODE-8435
> Project: Geode
> Issue Type: Bug
> Components: gfsh
> Reporter: Owen Nichols
> Assignee: Owen Nichols
> Priority: Major
> Labels: pull-request-available
>
> This used to work but was broken recently by changes for GEODE-8331.
> Although this does not impact Geode releases, it causes quite a headache for
> developers that don't typically pass a product version in their gradle build
> command.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
