[
https://issues.apache.org/jira/browse/GEODE-8435?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17181976#comment-17181976
]
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_r474792807
##########
File path:
geode-gfsh/src/main/java/org/apache/geode/management/internal/cli/commands/ConnectCommand.java
##########
@@ -197,9 +198,56 @@ public ResultModel connect(
}
}
- private static String versionComponent(String version, int component) {
+ private static String getRemoteSerializationVersion(OperationInvoker
invoker) {
+ try {
+ return invoker.getRemoteGeodeSerializationVersion();
+ } catch (Exception ignore) {
+ // expected to fail for Geode cluster older than 1.12
+ return null;
+ }
+ }
+
+ /**
+ * because remote serialization version was not exposed until 1.12, but we
are compatible back to
+ * 1.10, then any 1.x remote serialization version implies compatibility;
otherwise make some
+ * narrow assumptions about product version numbers known to be associated
with 1.10/1.11 to fill
+ * the gap.
+ *
+ * It seems reasonable to commit to future compatibility with any future
Geode of the same major,
+ * but we should probably draw the line somewhere and not promise that gfsh
1.x will be eternally
+ * compatible with Geode 2.x and later
+ */
+ static boolean shouldConnect(String ourSerializationVersion, String
remoteVersion,
Review comment:
It's great that we now have a method that encodes the logic but does no
I/O!
It's a minor issue that this method requires the caller to always (try to)
retrieve the `remoteVersion` even though, if `remoteSerializationVersion` was
retrieved then `remoteVersion` is not necessary.
This could be addressed by passing a `Supplier<String>
remoteVersionSupplier`. On the other hand it's not a big deal and this PR is
approved without that change.
##########
File path:
geode-gfsh/src/test/java/org/apache/geode/management/internal/cli/commands/ConnectCommandTest.java
##########
@@ -362,28 +362,40 @@ public void resolveSslProperties() {
}
@Test
- public void connectToManagerWithDifferentMajorVersion() {
+ public void connectToManagerWithOlderMajorVersionAllowed() {
when(gfsh.getVersion()).thenReturn("2.2");
- when(operationInvoker.getRemoteVersion()).thenReturn("1.2");
+ when(operationInvoker.getRemoteVersion()).thenReturn("1.10");
+ when(gfsh.getGeodeSerializationVersion()).thenReturn("2.2");
+
when(operationInvoker.getRemoteGeodeSerializationVersion()).thenReturn("1.10");
when(operationInvoker.isConnected()).thenReturn(true);
+
Review comment:
Big improvement in these test methods!
Now that they are setting "all the things" there is an opportunity to make
these test cases more understandable by factoring out the when-then statements
into a separate method. That would take away the visual noise and allow
maintainers to see more clearly what each test case is really doing—similar to
the tests for `ConnectCommand.shouldConnect()` below!
These methods work though so they are acceptable as-is.
##########
File path:
geode-gfsh/src/main/java/org/apache/geode/management/internal/cli/commands/ConnectCommand.java
##########
@@ -197,9 +198,56 @@ public ResultModel connect(
}
}
- private static String versionComponent(String version, int component) {
+ private static String getRemoteSerializationVersion(OperationInvoker
invoker) {
+ try {
+ return invoker.getRemoteGeodeSerializationVersion();
+ } catch (Exception ignore) {
+ // expected to fail for Geode cluster older than 1.12
+ return null;
+ }
+ }
+
+ /**
+ * because remote serialization version was not exposed until 1.12, but we
are compatible back to
+ * 1.10, then any 1.x remote serialization version implies compatibility;
otherwise make some
+ * narrow assumptions about product version numbers known to be associated
with 1.10/1.11 to fill
+ * the gap.
+ *
+ * It seems reasonable to commit to future compatibility with any future
Geode of the same major,
+ * but we should probably draw the line somewhere and not promise that gfsh
1.x will be eternally
+ * compatible with Geode 2.x and later
+ */
+ static boolean shouldConnect(String ourSerializationVersion, String
remoteVersion,
+ String remoteSerializationVersion) {
+ // pre 1.5
+ if (remoteVersion == null) {
+ return false;
+ }
Review comment:
This method would be easier to analyze if this check on `remoteVersion`
was moved down with the other checks on `remoteVersion` (after the checks on
`remoteSerializationVersion`.
Notice the form of the check on `remoteSerializationVersion` that follows.
There's a null-check guarding it. Doing the same for `remoteVersion` would make
the checks symmetrical.
It's acceptable as-is though.
----------------------------------------------------------------
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)
