[GitHub] [geode] DonalEvans commented on a change in pull request #6050: GEODE-8963: separate client/server compatibility from server/server version compatibility

[GitBox <gi...@apache.org>]

DonalEvans commented on a change in pull request #6050:
URL: https://github.com/apache/geode/pull/6050#discussion_r581454696



##########
File path: geode-cq/src/upgradeTest/java/org/apache/geode/internal/cache/tier/sockets/ClientServerMiscBCDUnitTest.java
##########
@@ -79,11 +84,49 @@ public ClientServerMiscBCDUnitTest(String version) {
   void createClientCacheAndVerifyPingIntervalIsSet(String host, int port) throws Exception {
     // this functionality was introduced in 1.5. If we let the test run in older
     // clients it will throw a NoSuchMethodError
-    if (VersionManager.getInstance().getCurrentVersionOrdinal() >= 80 /* GEODE_1_5_0 */) {
+    if (VersionManager.getInstance().getCurrentVersionOrdinal() >= KnownVersion.GEODE_1_5_0
+        .ordinal()) {
       super.createClientCacheAndVerifyPingIntervalIsSet(host, port);
     }
   }
 
+  /**
+   * A client should advertise its protocol version, which may not be the same
+   * as its product version.
+   */
+  @Test
+  public void testClientProtocolVersion() throws Exception {

Review comment:
       This test never throws an Exception, so this can be omitted.

##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/tier/ConnectionProxy.java
##########
@@ -19,17 +19,9 @@
 
 /**
  * Provides the version of the client.
- *
- * @since GemFire 2.0.2
  */
 @SuppressWarnings("deprecation")

Review comment:
       This suppression is no longer needed, as no deprecated code is used in this class

##########
File path: geode-cq/src/upgradeTest/java/org/apache/geode/internal/cache/tier/sockets/ClientServerMiscBCDUnitTest.java
##########
@@ -79,11 +84,49 @@ public ClientServerMiscBCDUnitTest(String version) {
   void createClientCacheAndVerifyPingIntervalIsSet(String host, int port) throws Exception {
     // this functionality was introduced in 1.5. If we let the test run in older
     // clients it will throw a NoSuchMethodError
-    if (VersionManager.getInstance().getCurrentVersionOrdinal() >= 80 /* GEODE_1_5_0 */) {
+    if (VersionManager.getInstance().getCurrentVersionOrdinal() >= KnownVersion.GEODE_1_5_0
+        .ordinal()) {
       super.createClientCacheAndVerifyPingIntervalIsSet(host, port);
     }
   }
 
+  /**
+   * A client should advertise its protocol version, which may not be the same
+   * as its product version.
+   */
+  @Test
+  public void testClientProtocolVersion() throws Exception {
+    int serverPort = initServerCache(true);
+    VM client1 = Host.getHost(0).getVM(testVersion, 1);
+    String hostname = NetworkUtils.getServerHostName(Host.getHost(0));
+    short ordinal = client1.invoke("create client1 cache", () -> {
+      createClientCache(hostname, serverPort);
+      populateCache();
+      registerInterest();
+      InternalDistributedMember distributedMember = (InternalDistributedMember) static_cache
+          .getDistributedSystem().getDistributedMember();
+      // older versions of Geode have a different Version class so we have to use reflection here
+      try {
+        Method getter = InternalDistributedMember.class.getMethod("getVersionObject");
+        Object versionObject = getter.invoke(distributedMember);
+        Method getOrdinal = versionObject.getClass().getMethod("ordinal");
+        return (Short) getOrdinal.invoke(versionObject);
+      } catch (NoSuchMethodException e) {

Review comment:
       The compiler warning here can be fixed by naming the caught exception "ignore" instead of e.

##########
File path: geode-cq/src/upgradeTest/java/org/apache/geode/internal/cache/tier/sockets/ClientServerMiscBCDUnitTest.java
##########
@@ -79,11 +84,49 @@ public ClientServerMiscBCDUnitTest(String version) {
   void createClientCacheAndVerifyPingIntervalIsSet(String host, int port) throws Exception {
     // this functionality was introduced in 1.5. If we let the test run in older
     // clients it will throw a NoSuchMethodError
-    if (VersionManager.getInstance().getCurrentVersionOrdinal() >= 80 /* GEODE_1_5_0 */) {
+    if (VersionManager.getInstance().getCurrentVersionOrdinal() >= KnownVersion.GEODE_1_5_0
+        .ordinal()) {
       super.createClientCacheAndVerifyPingIntervalIsSet(host, port);
     }
   }
 
+  /**
+   * A client should advertise its protocol version, which may not be the same
+   * as its product version.
+   */
+  @Test
+  public void testClientProtocolVersion() throws Exception {
+    int serverPort = initServerCache(true);
+    VM client1 = Host.getHost(0).getVM(testVersion, 1);
+    String hostname = NetworkUtils.getServerHostName(Host.getHost(0));

Review comment:
       `getServerHostName(Host host)` is deprecated and should be replaced with `getServerHostName()`.




----------------------------------------------------------------
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:
users@infra.apache.org