onichols-pivotal commented on a change in pull request #5462:
URL: https://github.com/apache/geode/pull/5462#discussion_r475019758
##########
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 ordering was requested by a previous review comment. many
orderings are possible but I happen to like this one because the checks are
ordered from simplest and most common first to trickiest edge case last, while
keeping indentation and flow control to a minimum.
----------------------------------------------------------------
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
