[GitHub] [geode] kirklund commented on a change in pull request #5645: GEODE-7845: Now behaving with clients of various versions.

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

kirklund commented on a change in pull request #5645:
URL: https://github.com/apache/geode/pull/5645#discussion_r509462790



##########
File path: geode-core/src/upgradeTest/java/org/apache/geode/internal/cache/rollingupgrade/RollingUpgradePartitionRegionClearServerVersionMismatch.java
##########
@@ -109,20 +111,28 @@ public void before() {
       region.put("A", "ValueA");
       region.put("B", "ValueB");
     });
+  }
+
+  @After
+  public void after() {
+    locator.stop();
+    serverNew.stop();
+    serverOld.stop();
 
   }
 
   /**
-   * testClient_ServerVersionMismatchException - validates that when a client invokes a partitioned
-   * region clear on a cluster where one server is running an unsupported version for this feature
-   * we return a ServerVersionMismatchException
+   * testClient_ServerOperationException - validates that when a client invokes a partitioned region
+   * clear on a cluster where one server is running an unsupported version for this feature we
+   * return a ServerOperationException
    */
   @Test
-  public void testClient_ServerVersionMismatchException() throws Exception {
+  public void testClient_ServerOperationExceptionCurrentServerVersion() throws Exception {
     IgnoredException.addIgnoredException(ServerOperationException.class);
-    final int locatorPort = locator.getPort();
+
     // Get a client VM
-    ClientVM clientVM = cluster.startClientVM(3, c -> c.withLocatorConnection(locatorPort));
+    int serverPort = serverNew.getPort();
+    clientVM = cluster.startClientVM(3, oldVersion, c -> c.withServerConnection(serverPort));

Review comment:
       I think Gester is recommending expanding the testing to include:
   
   1. new client --> new server
   2. new client --> old server
   3. old client --> new server
   4. old client --> old server
   
   If (3) and (4) throw UnsupportedOperationException without sending any message to the server, then they could probably be collapsed into a single test.

##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/PartitionedRegionClear.java
##########
@@ -349,13 +348,14 @@ public void allServerVersionsSupportPartitionRegionClear() {
           && (internalDistributedMember.getVersion().isOlderThan(KnownVersion.GEODE_1_14_0))) {
         if (!memberNames.contains(internalDistributedMember.getName())) {
           memberNames.add(internalDistributedMember.getName());
-          logger.info("MLH adding " + internalDistributedMember.getName());
         }
       }
     }
     if (!memberNames.isEmpty()) {
-      throw new ServerVersionMismatchException(memberNames, "Partitioned Region Clear",
-          KnownVersion.GEODE_1_14_0.toString());
+      throw new UnsupportedOperationException(
+          "A server's " + memberNames + " version was too old (< "

Review comment:
       This message reads weird. Maybe change to:
   ```
   "Server(s) " + memberNames + " are running a version older than (<" + KnownVersion.GEODE_1_14_0 + ">) which does not support partitioned region clear.");
   ```




----------------------------------------------------------------
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