Refactor AsyncRequestResponseHandlerFactory (#1356)

Unify rest.server.request.handler.factory and rest.server.response.handler.factory
Refactor AsyncRequestResponseHandlerFactory to factory pattern from singleton pattern
Add setupResponseHandler for RestRequestService.
Remove setupRequestHandling from AsyncRequestResponseHandler.
A regular order to start rest handler would be:
1. Create RestRequestService
2. Create RequestResponseHandler based on RestRequestService
3. Setup ResponseHandler for RestRequestService

ambry-api/src/main/java/com.github.ambry/config/RestServerConfig.java

@@ -40,12 +40,12 @@ public class RestServerConfig {
   public final int restServerRequestHandlerScalingUnitCount;
 
   /**
-   * The RestRequestHandlerFactory that needs to be used by the RestServer
-   * for bootstrapping the RestRequestHandler
+   * The RequestResponseHandlerFactory that needs to be used by the RestServer or AmbryServer HTTP2
+   * for bootstrapping the RestRequestHandler and RestResponseHandler
    */
-  @Config("rest.server.request.handler.factory")
+  @Config("rest.server.request.response.handler.factory")
   @Default("com.github.ambry.rest.AsyncRequestResponseHandlerFactory")
-  public final String restServerRequestHandlerFactory;
+  public final String restServerRequestResponseHandlerFactory;
 
   /**
    * The number of scaling units in RestResponseHandler handle responses.
@@ -54,14 +54,6 @@ public class RestServerConfig {
   @Default("5")
   public final int restServerResponseHandlerScalingUnitCount;
 
-  /**
-   * The RestResponseHandlerFactory that needs to be used by the RestServer
-   * for bootstrapping the RestResponseHandler.
-   */
-  @Config("rest.server.response.handler.factory")
-  @Default("com.github.ambry.rest.AsyncRequestResponseHandlerFactory")
-  public final String restServerResponseHandlerFactory;
-
   /**
    * The AccountServiceFactory that needs to be used by FrontendRestRequestService to get account-related information.
    */
@@ -100,16 +92,14 @@ public class RestServerConfig {
 
   public RestServerConfig(VerifiableProperties verifiableProperties) {
     restServerRestRequestServiceFactory = verifiableProperties.getString("rest.server.rest.request.service.factory");
-    restServerNioServerFactory = verifiableProperties.getString("rest.server.nio.server.factory",
-        "com.github.ambry.rest.FrontendNettyFactory");
+    restServerNioServerFactory =
+        verifiableProperties.getString("rest.server.nio.server.factory", "com.github.ambry.rest.FrontendNettyFactory");
     restServerRequestHandlerScalingUnitCount =
         verifiableProperties.getIntInRange("rest.server.request.handler.scaling.unit.count", 5, 0, Integer.MAX_VALUE);
-    restServerRequestHandlerFactory = verifiableProperties.getString("rest.server.request.handler.factory",
+    restServerRequestResponseHandlerFactory = verifiableProperties.getString("rest.server.request.response.handler.factory",
         "com.github.ambry.rest.AsyncRequestResponseHandlerFactory");
     restServerResponseHandlerScalingUnitCount =
         verifiableProperties.getIntInRange("rest.server.response.handler.scaling.unit.count", 5, 0, Integer.MAX_VALUE);
-    restServerResponseHandlerFactory = verifiableProperties.getString("rest.server.response.handler.factory",
-        "com.github.ambry.rest.AsyncRequestResponseHandlerFactory");
     restServerAccountServiceFactory = verifiableProperties.getString("rest.server.account.service.factory",
         "com.github.ambry.account.InMemoryUnknownAccountServiceFactory");
     restServerRouterFactory = verifiableProperties.getString("rest.server.router.factory",

ambry-api/src/main/java/com.github.ambry/rest/RestResponseHandlerFactory.java → ambry-api/src/main/java/com.github.ambry/rest/RestRequestResponseHandlerFactory.java

@@ -14,12 +14,18 @@
 package com.github.ambry.rest;
 
 /**
- * RestResponseHandlerFactory is a factory to generate all the supporting cast required to instantiate a
- * {@link RestResponseHandler}.
- * <p/>
+ * RestRequestResponseHandlerFactory is a factory to generate all the supporting cast required to instantiate
+ * {@link RestRequestHandler} and {@link RestResponseHandler}.
  * Usually called with the canonical class name and as such might have to support appropriate (multiple) constructors.
  */
-public interface RestResponseHandlerFactory {
+public interface RestRequestResponseHandlerFactory {
+
+  /**
+   * Returns an instance of the {@link RestRequestHandler} that the factory generates.
+   * @return an instance of {@link RestRequestHandler} generated by this factory.
+   * @throws InstantiationException if the {@link RestRequestHandler} instance cannot be created.
+   */
+  public RestRequestHandler getRestRequestHandler() throws InstantiationException;
 
   /**
    * Returns an instance of the {@link RestResponseHandler} that the factory generates.

ambry-api/src/main/java/com.github.ambry/rest/RestRequestService.java

@@ -24,6 +24,13 @@
  */
 public interface RestRequestService {
 
+  /**
+   * Setup {@link RestResponseHandler} for this {@link RestRequestService}.
+   * This method should be called before {@link RestRequestService#start()}
+   * @param responseHandler the {@link RestResponseHandler} that can be used to submit responses.
+   */
+  public void setupResponseHandler(RestResponseHandler responseHandler);
+
   /**
    * Does startup tasks for the RestRequestService. When the function returns, startup is FULLY complete.
    * @throws InstantiationException if RestRequestService is unable to start.

ambry-api/src/test/java/com.github.ambry/rest/MockRestRequestResponseHandler.java

@@ -40,7 +40,12 @@ public class MockRestRequestResponseHandler implements RestRequestHandler, RestR
   private boolean isRunning = false;
   private VerifiableProperties failureProperties = null;
 
-  private RestRequestService restRequestService = null;
+  private final RestRequestService restRequestService;
+
+  public MockRestRequestResponseHandler(RestRequestService restRequestService) {
+    this.restRequestService = restRequestService;
+    this.restRequestService.setupResponseHandler(this);
+  }
 
   @Override
   public void start() throws InstantiationException {
@@ -142,14 +147,6 @@ public void fix() {
     failureProperties = null;
   }
 
-  /**
-   * Sets the {@link RestRequestService} that will be used.
-   * @param restRequestService the {@link RestRequestService} instance to be used to process requests.
-   */
-  protected void setRestRequestService(RestRequestService restRequestService) {
-    this.restRequestService = restRequestService;
-  }
-
   /**
    * If the MockRestRequestResponseHandler is supposed to be breakdown, throws the right exception.
    * @param restRequest the {@link RestRequest} that needs to be handled.

ambry-api/src/test/java/com.github.ambry/rest/MockRestRequestResponseHandlerFactory.java

@@ -13,56 +13,33 @@
  */
 package com.github.ambry.rest;
 
-import java.util.concurrent.atomic.AtomicBoolean;
-
-
 /**
- * Implementation of {@link RestRequestHandlerFactory} and {@link RestResponseHandlerFactory} that can be used in tests.
- * <p/>
+ * Implementation of {@link RestRequestResponseHandlerFactory} that can be used in tests.
  * Sets up all the supporting cast required for {@link MockRestRequestResponseHandler}. Maintains a single instance of
  * {@link MockRestRequestResponseHandler} and returns the same instance on any call to {@link #getRestRequestHandler()} or
  * {@link #getRestResponseHandler()}.
  */
-public class MockRestRequestResponseHandlerFactory implements RestRequestHandlerFactory, RestResponseHandlerFactory {
-  private static final AtomicBoolean instantiated = new AtomicBoolean(false);
-  private static MockRestRequestResponseHandler instance;
-
-  public MockRestRequestResponseHandlerFactory(Object handlerCount, Object metricRegistry) {
-  }
+public class MockRestRequestResponseHandlerFactory implements RestRequestResponseHandlerFactory {
+  private MockRestRequestResponseHandler handler;
 
   public MockRestRequestResponseHandlerFactory(Object handlerCount, Object metricRegistry,
       RestRequestService restRequestService) {
-    MockRestRequestResponseHandler requestHandler = getInstance();
-    requestHandler.setRestRequestService(restRequestService);
+    handler = new MockRestRequestResponseHandler(restRequestService);
   }
 
   /**
-   * Returns an instance of {@link MockRestRequestResponseHandler}.
    * @return an instance of {@link MockRestRequestResponseHandler}.
    */
   @Override
   public RestRequestHandler getRestRequestHandler() throws InstantiationException {
-    return getInstance();
+    return handler;
   }
 
   /**
-   * Returns an instance of {@link MockRestRequestResponseHandler}.
    * @return an instance of {@link MockRestRequestResponseHandler}.
    */
   @Override
   public RestResponseHandler getRestResponseHandler() throws InstantiationException {
-    return getInstance();
-  }
-
-  /**
-   * Returns the singleton {@link MockRestRequestResponseHandler} instance being maintained. Creates it if it hasn't been
-   * created already.
-   * @return an instance of {@link MockRestRequestResponseHandler}.
-   */
-  private static MockRestRequestResponseHandler getInstance() {
-    if (instantiated.compareAndSet(false, true)) {
-      instance = new MockRestRequestResponseHandler();
-    }
-    return instance;
+    return handler;
   }
 }

ambry-api/src/test/java/com.github.ambry/rest/MockRestRequestService.java

@@ -46,7 +46,7 @@ public class MockRestRequestService implements RestRequestService {
   public final static String SEND_RESPONSE_REST_SERVICE_EXCEPTION = "mbssRestServiceException";
   public final static String REST_ERROR_CODE = "mock.rest.request.service.rest.error.code";
 
-  private final RestResponseHandler responseHandler;
+  private RestResponseHandler responseHandler;
   private final Router router;
 
   private VerifiableProperties verifiableProperties;
@@ -67,18 +67,23 @@ public void setVerifiableProperties(VerifiableProperties verifiableProperties) {
    * Creates an instance of {@link MockRestRequestService} with {@code router} as the backing {@link Router} and
    * {@code verifiableProperties} defining the behavior of this instance.
    * @param verifiableProperties the {@link VerifiableProperties} that defines the behavior of this instance.
-   * @param responseHandler the {@link RestResponseHandler} instance to use.
    * @param router the {@link Router} that will back this instance.
    */
-  public MockRestRequestService(VerifiableProperties verifiableProperties, RestResponseHandler responseHandler,
-      Router router) {
+  public MockRestRequestService(VerifiableProperties verifiableProperties, Router router) {
     setVerifiableProperties(verifiableProperties);
-    this.responseHandler = responseHandler;
     this.router = router;
   }
 
+  @Override
+  public void setupResponseHandler(RestResponseHandler responseHandler) {
+    this.responseHandler = responseHandler;
+  }
+
   @Override
   public void start() throws InstantiationException {
+    if (responseHandler == null) {
+      throw new InstantiationException("ResponseHandler is not set.");
+    }
     serviceRunning = true;
   }
 

ambry-api/src/test/java/com.github.ambry/rest/MockRestRequestServiceFactory.java

@@ -27,13 +27,11 @@
  */
 public class MockRestRequestServiceFactory implements RestRequestServiceFactory {
   private final VerifiableProperties verifiableProperties;
-  private final RestResponseHandler restResponseHandler;
   private final Router router;
 
-  public MockRestRequestServiceFactory(VerifiableProperties verifiableProperties, ClusterMap clusterMap,
-      RestResponseHandler restResponseHandler, Router router, AccountService accountService) {
+  public MockRestRequestServiceFactory(VerifiableProperties verifiableProperties, ClusterMap clusterMap, Router router,
+      AccountService accountService) {
     this.verifiableProperties = verifiableProperties;
-    this.restResponseHandler = restResponseHandler;
     this.router = router;
   }
 
@@ -44,6 +42,6 @@ public MockRestRequestServiceFactory(VerifiableProperties verifiableProperties,
    */
   @Override
   public RestRequestService getRestRequestService() {
-    return new MockRestRequestService(verifiableProperties, restResponseHandler, router);
+    return new MockRestRequestService(verifiableProperties, router);
   }
 }

ambry-frontend/src/main/java/com.github.ambry.frontend/FrontendRestRequestService.java

@@ -68,7 +68,6 @@ class FrontendRestRequestService implements RestRequestService {
   private static final String OPERATION_TYPE_GET = "GET";
   private static final String OPERATION_TYPE_HEAD = "HEAD";
   private static final String OPERATION_TYPE_DELETE = "DELETE";
-  private final RestResponseHandler responseHandler;
   private final Router router;
   private final IdConverterFactory idConverterFactory;
   private final SecurityServiceFactory securityServiceFactory;
@@ -84,6 +83,7 @@ class FrontendRestRequestService implements RestRequestService {
   private final String datacenterName;
   private final String hostname;
   private final String clusterName;
+  private RestResponseHandler responseHandler;
   private IdConverter idConverter = null;
   private SecurityService securityService = null;
   private GetPeersHandler getPeersHandler;
@@ -100,7 +100,6 @@ class FrontendRestRequestService implements RestRequestService {
    * response handler controller and a router.
    * @param frontendConfig the {@link FrontendConfig} with configuration parameters.
    * @param frontendMetrics the metrics instance to use in the form of {@link FrontendMetrics}.
-   * @param responseHandler the {@link RestResponseHandler} that can be used to submit responses that need to be sent out.
    * @param router the {@link Router} instance to use to perform blob operations.
    * @param clusterMap the {@link ClusterMap} in use.
    * @param idConverterFactory the {@link IdConverterFactory} to use to get an {@link IdConverter} instance.
@@ -114,14 +113,13 @@ class FrontendRestRequestService implements RestRequestService {
    * @param clusterName the name of the storage cluster that the router communicates with.
    */
   FrontendRestRequestService(FrontendConfig frontendConfig, FrontendMetrics frontendMetrics,
-      RestResponseHandler responseHandler, Router router, ClusterMap clusterMap, IdConverterFactory idConverterFactory,
+      Router router, ClusterMap clusterMap, IdConverterFactory idConverterFactory,
       SecurityServiceFactory securityServiceFactory, UrlSigningService urlSigningService,
       IdSigningService idSigningService, AccountService accountService,
       AccountAndContainerInjector accountAndContainerInjector, String datacenterName, String hostname,
       String clusterName) {
     this.frontendConfig = frontendConfig;
     this.frontendMetrics = frontendMetrics;
-    this.responseHandler = responseHandler;
     this.router = router;
     this.clusterMap = clusterMap;
     this.idConverterFactory = idConverterFactory;
@@ -137,8 +135,19 @@ class FrontendRestRequestService implements RestRequestService {
     logger.trace("Instantiated FrontendRestRequestService");
   }
 
+  /**
+   * @param responseHandler the {@link RestResponseHandler} that can be used to submit responses that need to be sent out.
+   */
+  @Override
+  public void setupResponseHandler(RestResponseHandler responseHandler) {
+    this.responseHandler = responseHandler;
+  }
+
   @Override
   public void start() throws InstantiationException {
+    if (responseHandler == null) {
+      throw new InstantiationException("ResponseHandler is not set.");
+    }
     long startupBeginTime = System.currentTimeMillis();
     idConverter = idConverterFactory.getIdConverter();
     securityService = securityServiceFactory.getSecurityService();

ambry-frontend/src/main/java/com.github.ambry.frontend/FrontendRestRequestServiceFactory.java

@@ -20,7 +20,6 @@
 import com.github.ambry.config.VerifiableProperties;
 import com.github.ambry.rest.RestRequestService;
 import com.github.ambry.rest.RestRequestServiceFactory;
-import com.github.ambry.rest.RestResponseHandler;
 import com.github.ambry.router.Router;
 import com.github.ambry.utils.Utils;
 import java.util.Objects;
@@ -40,7 +39,6 @@ public class FrontendRestRequestServiceFactory implements RestRequestServiceFact
   private final VerifiableProperties verifiableProperties;
   private final ClusterMap clusterMap;
   private final ClusterMapConfig clusterMapConfig;
-  private final RestResponseHandler responseHandler;
   private final Router router;
   private final AccountService accountService;
   private final Logger logger = LoggerFactory.getLogger(getClass());
@@ -49,17 +47,14 @@ public class FrontendRestRequestServiceFactory implements RestRequestServiceFact
    * Creates a new instance of FrontendRestRequestServiceFactory.
    * @param verifiableProperties the properties to use to create configs.
    * @param clusterMap the {@link ClusterMap} to use.
-   * @param responseHandler the {@link RestResponseHandler} that can be used to submit responses that need to be sent
-   *                        out.
    * @param router the {@link Router} to use.
    * @param accountService the {@link AccountService} to use.
    * @throws IllegalArgumentException if any of the arguments are null.
    */
   public FrontendRestRequestServiceFactory(VerifiableProperties verifiableProperties, ClusterMap clusterMap,
-      RestResponseHandler responseHandler, Router router, AccountService accountService) {
+      Router router, AccountService accountService) {
     this.verifiableProperties = Objects.requireNonNull(verifiableProperties, "Provided VerifiableProperties is null");
     this.clusterMap = Objects.requireNonNull(clusterMap, "Provided ClusterMap is null");
-    this.responseHandler = Objects.requireNonNull(responseHandler, "Provided RestResponseHandler is null");
     this.router = Objects.requireNonNull(router, "Provided Router is null");
     this.accountService = Objects.requireNonNull(accountService, "Provided AccountService is null");
     clusterMapConfig = new ClusterMapConfig(verifiableProperties);
@@ -89,9 +84,9 @@ public RestRequestService getRestRequestService() {
       SecurityServiceFactory securityServiceFactory =
           Utils.getObj(frontendConfig.securityServiceFactory, verifiableProperties, clusterMap, accountService,
               urlSigningService, idSigningService, accountAndContainerInjector);
-      return new FrontendRestRequestService(frontendConfig, frontendMetrics, responseHandler, router, clusterMap,
-          idConverterFactory, securityServiceFactory, urlSigningService, idSigningService, accountService,
-          accountAndContainerInjector, clusterMapConfig.clusterMapDatacenterName, clusterMapConfig.clusterMapHostName,
+      return new FrontendRestRequestService(frontendConfig, frontendMetrics, router, clusterMap, idConverterFactory,
+          securityServiceFactory, urlSigningService, idSigningService, accountService, accountAndContainerInjector,
+          clusterMapConfig.clusterMapDatacenterName, clusterMapConfig.clusterMapHostName,
           clusterMapConfig.clusterMapClusterName);
     } catch (Exception e) {
       throw new IllegalStateException("Could not instantiate FrontendRestRequestService", e);