GEODE-3465: Enhance error message and add tests for when group is spe…

…cified. (apache#1527)
ybz216 · Mar 2, 2018 · 5380ed5 · 5380ed5
1 parent b9d9b38
commit 5380ed5
Show file tree

Hide file tree

Showing 3 changed files with 74 additions and 25 deletions.
diff --git a/.../org/apache/geode/internal/protocol/protobuf/v1/operations/GetServerOperationHandler.java b/.../org/apache/geode/internal/protocol/protobuf/v1/operations/GetServerOperationHandler.java
@@ -19,6 +19,7 @@
 
 import java.util.HashSet;
 import java.util.List;
+import java.util.Objects;
 import java.util.Set;
 
 import org.apache.geode.annotations.Experimental;
@@ -69,13 +70,17 @@ public Result<LocatorAPI.GetServerResponse> process(
         .getServerLocatorAdvisee().processRequest(clientConnectionRequest);
 
     ServerLocation serverLocation = null;
-    if (connectionResponse != null) {
+    if (connectionResponse != null && connectionResponse.hasResult()) {
       serverLocation = connectionResponse.getServer();
     }
 
     if (serverLocation == null) {
-      return Failure.of(NO_AVAILABLE_SERVER, "Unable to find a server for you");
-
+      StringBuilder builder = new StringBuilder("Unable to find a server");
+      if (!Objects.isNull(serverGroup) && !serverGroup.isEmpty()) {
+        builder.append(" in server group ");
+        builder.append(serverGroup);
+      }
+      return Failure.of(NO_AVAILABLE_SERVER, builder.toString());
     } else {
       LocatorAPI.GetServerResponse.Builder builder = LocatorAPI.GetServerResponse.newBuilder();
       BasicTypes.Server.Builder serverBuilder = BasicTypes.Server.newBuilder();

diff --git a/...va/org/apache/geode/internal/protocol/protobuf/v1/utilities/ProtobufRequestUtilities.java b/...va/org/apache/geode/internal/protocol/protobuf/v1/utilities/ProtobufRequestUtilities.java
@@ -109,8 +109,25 @@ public static ClientProtocol.Message createPutAllRequest(String regionName,
     return ClientProtocol.Message.newBuilder().setPutAllRequest(putAllRequestBuilder).build();
   }
 
+  /**
+   * Create a request to retrieve a server.
+   *
+   * @return Request object containing the get-server request.
+   */
   public static LocatorAPI.GetServerRequest createGetServerRequest() {
     LocatorAPI.GetServerRequest.Builder builder = LocatorAPI.GetServerRequest.newBuilder();
     return builder.build();
   }
+
+  /**
+   * Create a request to retrieve a server from a server group.
+   *
+   * @param serverGroup Server group name.
+   * @return Request object containing the get-server request.
+   */
+  public static LocatorAPI.GetServerRequest createGetServerRequest(String serverGroup) {
+    LocatorAPI.GetServerRequest.Builder builder = LocatorAPI.GetServerRequest.newBuilder();
+    builder.setServerGroup(serverGroup);
+    return builder.build();
+  }
 }
diff --git a/...he/geode/internal/protocol/protobuf/v1/operations/GetServerOperationHandlerJUnitTest.java b/...he/geode/internal/protocol/protobuf/v1/operations/GetServerOperationHandlerJUnitTest.java
@@ -16,6 +16,7 @@
 
 import static org.apache.geode.internal.protocol.protobuf.v1.BasicTypes.ErrorCode.NO_AVAILABLE_SERVER;
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertTrue;
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.Mockito.mock;
@@ -25,6 +26,7 @@
 import org.junit.Test;
 import org.junit.experimental.categories.Category;
 
+import org.apache.geode.cache.client.internal.locator.ClientConnectionRequest;
 import org.apache.geode.cache.client.internal.locator.ClientConnectionResponse;
 import org.apache.geode.distributed.internal.InternalLocator;
 import org.apache.geode.distributed.internal.ServerLocation;
@@ -42,18 +44,15 @@
 
 @Category(UnitTest.class)
 public class GetServerOperationHandlerJUnitTest extends OperationHandlerJUnitTest {
-
-  private final String HOSTNAME_1 = "hostname1";
-  private final int PORT_1 = 12345;
-
-  private final String HOSTNAME_2 = "hostname2";
-  private final int PORT_2 = 23456;
-
-  private InternalLocator internalLocatorMock;
+  final String HOSTNAME = "hostname";
+  final int PORT = 12345;
+  final String EXISTENT_GROUP = "existent";
+  final String NONEXISTENT_GROUP = "nonexistent";
+  InternalLocator internalLocatorMock;
   ServerLocator serverLocatorAdviseeMock;
 
   @Before
-  public void setUp() throws Exception {
+  public void setUp() {
     operationHandler = new GetServerOperationHandler();
     internalLocatorMock = mock(InternalLocator.class);
     serverLocatorAdviseeMock = mock(ServerLocator.class);
@@ -64,27 +63,55 @@ public void setUp() throws Exception {
   @Test
   public void testServerReturnedFromHandler() throws Exception {
     when(serverLocatorAdviseeMock.processRequest(any(Object.class)))
-        .thenReturn(new ClientConnectionResponse(new ServerLocation(HOSTNAME_1, PORT_1)));
+        .thenReturn(new ClientConnectionResponse(new ServerLocation(HOSTNAME, PORT)));
 
-    LocatorAPI.GetServerRequest GetServerRequest =
+    LocatorAPI.GetServerRequest getServerRequest =
         ProtobufRequestUtilities.createGetServerRequest();
-    Result operationHandlerResult = getOperationHandlerResult(GetServerRequest);
+    Result operationHandlerResult = getOperationHandlerResult(getServerRequest);
     assertTrue(operationHandlerResult instanceof Success);
     validateGetServerResponse((GetServerResponse) operationHandlerResult.getMessage());
   }
 
   @Test
-  public void testExceptionReturnedWhenNoServers() throws Exception {
+  public void testErrorReturnedWhenNoServers() throws Exception {
     when(serverLocatorAdviseeMock.processRequest(any(Object.class))).thenReturn(null);
 
-    LocatorAPI.GetServerRequest GetServerRequest =
+    LocatorAPI.GetServerRequest getServerRequest =
         ProtobufRequestUtilities.createGetServerRequest();
-    Result operationHandlerResult = getOperationHandlerResult(GetServerRequest);
+    Result operationHandlerResult = getOperationHandlerResult(getServerRequest);
+    assertTrue(operationHandlerResult instanceof Failure);
+    Failure failure = (Failure) operationHandlerResult;
+    ClientProtocol.ErrorResponse errorResponse = failure.getErrorMessage();
+    assertEquals(NO_AVAILABLE_SERVER, errorResponse.getError().getErrorCode());
+  }
+
+  @Test
+  public void testServerReturnedForExistentGroup() throws Exception {
+    when(
+        serverLocatorAdviseeMock.processRequest(new ClientConnectionRequest(any(), EXISTENT_GROUP)))
+            .thenReturn(new ClientConnectionResponse(new ServerLocation(HOSTNAME, PORT)));
+
+    LocatorAPI.GetServerRequest getServerRequest =
+        ProtobufRequestUtilities.createGetServerRequest(EXISTENT_GROUP);
+    Result operationHandlerResult = getOperationHandlerResult(getServerRequest);
+    assertTrue(operationHandlerResult instanceof Success);
+    validateGetServerResponse((GetServerResponse) operationHandlerResult.getMessage());
+  }
+
+  @Test
+  public void testErrorReturnedForNonexistentGroup() throws Exception {
+    when(serverLocatorAdviseeMock
+        .processRequest(new ClientConnectionRequest(any(), NONEXISTENT_GROUP)))
+            .thenReturn(new ClientConnectionResponse(null));
+
+    LocatorAPI.GetServerRequest getServerRequest =
+        ProtobufRequestUtilities.createGetServerRequest(NONEXISTENT_GROUP);
+    Result operationHandlerResult = getOperationHandlerResult(getServerRequest);
     assertTrue(operationHandlerResult instanceof Failure);
     Failure failure = (Failure) operationHandlerResult;
-    ClientProtocol.ErrorResponse errorResponse =
-        (ClientProtocol.ErrorResponse) failure.getErrorMessage();
+    ClientProtocol.ErrorResponse errorResponse = failure.getErrorMessage();
     assertEquals(NO_AVAILABLE_SERVER, errorResponse.getError().getErrorCode());
+    assertTrue(errorResponse.getError().getMessage().contains(NONEXISTENT_GROUP));
   }
 
   private Result getOperationHandlerResult(LocatorAPI.GetServerRequest GetServerRequest)
@@ -93,10 +120,10 @@ private Result getOperationHandlerResult(LocatorAPI.GetServerRequest GetServerRe
         TestExecutionContext.getLocatorExecutionContext(internalLocatorMock));
   }
 
-  private void validateGetServerResponse(GetServerResponse GetServerResponse) {
-    assertTrue(GetServerResponse.hasServer());
-    BasicTypes.Server server = GetServerResponse.getServer();
-    assertEquals(HOSTNAME_1, server.getHostname());
-    assertEquals(PORT_1, server.getPort());
+  private void validateGetServerResponse(GetServerResponse getServerResponse) {
+    assertTrue(getServerResponse.hasServer());
+    BasicTypes.Server server = getServerResponse.getServer();
+    assertEquals(HOSTNAME, server.getHostname());
+    assertEquals(PORT, server.getPort());
   }
 }