Refactor AsyncRequestResponseHandlerFactory #1356

zzmao · 2020-01-10T23:15:08Z

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:

Create RestRequestService
Create RequestResponseHandler based on RestRequestService
Setup ResponseHandler for RestRequestService

Unify rest.server.request.handler.factory and rest.server.response.handler.factory Refactor AsyncRequestResponseHandlerFactory to factory pattern from singleton pattern Add setupResponseHandler for RestRquestService. Remove setupRequestHandling from AsyncRequestResponseHandler.

codecov-io · 2020-01-10T23:37:49Z

Codecov Report

❗ No coverage uploaded for pull request base (master@c802113). Click here to learn what that means.
The diff coverage is 92.45%.

@@            Coverage Diff            @@
##             master    #1356   +/-   ##
=========================================
  Coverage          ?   72.05%           
  Complexity        ?     6612           
=========================================
  Files             ?      480           
  Lines             ?    37585           
  Branches          ?     4753           
=========================================
  Hits              ?    27082           
  Misses            ?     9232           
  Partials          ?     1271

Impacted Files	Coverage Δ	Complexity Δ
...github.ambry.server/StorageRestRequestService.java	`0% <0%> (ø)`	`0 <0> (?)`
...java/com.github.ambry/config/RestServerConfig.java	`100% <100%> (ø)`	`1 <0> (?)`
...ry.frontend/FrontendRestRequestServiceFactory.java	`92% <100%> (ø)`	`2 <0> (?)`
...ambry.rest/AsyncRequestResponseHandlerFactory.java	`100% <100%> (ø)`	`6 <2> (?)`
...hub.ambry.frontend/FrontendRestRequestService.java	`92.39% <100%> (ø)`	`66 <3> (?)`
...rc/main/java/com.github.ambry.rest/RestServer.java	`90.96% <90.9%> (ø)`	`12 <0> (?)`
...github.ambry.rest/AsyncRequestResponseHandler.java	`87.87% <93.1%> (ø)`	`21 <0> (?)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c802113...f401d23. Read the comment docs.

cgtz · 2020-01-11T00:04:26Z

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

   */
-  @Config("rest.server.request.handler.factory")
+  @Config("rest.server.request.response.handler.factory")
  @Default("com.github.ambry.rest.AsyncRequestResponseHandlerFactory")
  public final String restServerRequestHandlerFactory;


change variable name to restServerRequestResponseHandlerFactory

cgtz · 2020-01-11T00:14:14Z

ambry-rest/src/main/java/com.github.ambry.rest/AsyncRequestResponseHandler.java

    this.metrics = metrics;
    metrics.trackAsyncRequestResponseHandler(this);
+    if (isRunning()) {


Could this condition ever occur since this is logic is now in the constructor and isRunning is a non-static member?

It won't happen. Removing.

cgtz · 2020-01-11T00:18:57Z

ambry-rest/src/main/java/com.github.ambry.rest/AsyncRequestResponseHandlerFactory.java


+  private AsyncRequestResponseHandler handler;


could be final, I believe

cgtz · 2020-01-11T00:27:31Z

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

@@ -24,6 +24,11 @@
 */
 public interface RestRequestService {

+  /**


Add that this method must be called before the service is started

cgtz · 2020-01-11T00:34:17Z

I reviewed the changes in the second commit

zzmao · 2020-01-11T01:30:33Z

This PR is now independent.

jsjtzyy · 2020-01-13T18:43:02Z

ambry-rest/src/main/java/com.github.ambry.rest/RestServer.java

@@ -233,7 +235,7 @@ public void start() throws InstantiationException {
      restResponseHandler.start();


Had an offline discussion with Ze. restResponseHandler and restRequestHandler are actually same instance within restHandlerFactory. Hence, calling start() once should suffice. Let's leave a comment here to explain we removed restRequestHandler.start();. In the future, we probably could unify them.

jsjtzyy · 2020-01-13T19:00:38Z

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

  @Override
  public void start() throws InstantiationException {
+    if (responseHandler == null) {
+      throw new InstantiationException("ResponseHandler is not set.");


Feel like this could be an IllegalStateException

jsjtzyy

LGTM after a few minor comments are addressed

jsjtzyy · 2020-01-13T19:24:40Z

ambry-rest/src/main/java/com.github.ambry.rest/AsyncRequestResponseHandler.java

-    this.restRequestService = restRequestService;
-    logger.trace("Request handling units count set to {}", requestWorkersCount);
-  }
-


minor: line 268, could you removing the leading underscore of _restRequestService ?

jsjtzyy · 2020-01-13T19:30:51Z

ambry-rest/src/main/java/com.github.ambry.rest/AsyncRequestResponseHandlerFactory.java

    }
    logger.trace("Instantiated AsyncRequestResponseHandlerFactory as RestRequestHandler");
  }

  /**
-   * Returns an instance of {@link AsyncRequestResponseHandler}.
-   * @return an instance of {@link AsyncRequestResponseHandler}.
+   * @return an handler of {@link AsyncRequestResponseHandler}.


minor typo: return a handler or return an instance

jsjtzyy · 2020-01-13T19:30:58Z

ambry-rest/src/main/java/com.github.ambry.rest/AsyncRequestResponseHandlerFactory.java

  }

  /**
-   * Returns an instance of {@link AsyncRequestResponseHandler}.
-   * @return an instance of {@link AsyncRequestResponseHandler}.
+   * @return an handler of {@link AsyncRequestResponseHandler}.


jsjtzyy · 2020-01-13T19:34:23Z

ambry-rest/src/main/java/com.github.ambry.rest/AsyncRequestResponseHandlerFactory.java

@@ -171,7 +124,7 @@ private static void buildInstance(MetricRegistry metricRegistry) {
  public final Counter residualResponseSetSize;

  /**
-   * Creates an instance of RequestResponseHandlerMetrics using the given {@code metricRegistry}.
+   * Creates an handler of RequestResponseHandlerMetrics using the given {@code metricRegistry}.


minor: no need to change previous comment.

jsjtzyy · 2020-01-13T19:37:00Z

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

+  private final RestRequestService restRequestService;
+
+  public MockRestRequestResponseHandler(RestRequestService restRequestService) {
+    this.restRequestService = restRequestService;


do we need to check if restRequestService is null?

Since it's mock, I think it's ok to throw null exception.

jsjtzyy · 2020-01-13T20:42:55Z

ambry-rest/src/test/java/com.github.ambry.rest/AsyncRequestResponseHandlerTest.java

-      requestResponseHandler.setupRequestHandling(1, null);
-      fail("Setting RestRequestService to null should have thrown exception");
+      requestResponseHandler = getAsyncRequestResponseHandler(-1);
+      fail("Setting request workers < 0 should have thrown exception");


what's the difference between line 165 and line 173 test cases ?

zzmao requested review from cgtz and jsjtzyy January 10, 2020 23:15

cgtz approved these changes Jan 11, 2020

View reviewed changes

address cgetz comment

2d02ecc

zzmao force-pushed the nettyPRAsyncRR branch from 8ec518e to 2d02ecc Compare January 11, 2020 01:29

fix test

37f67c3

jsjtzyy reviewed Jan 13, 2020

View reviewed changes

jsjtzyy approved these changes Jan 13, 2020

View reviewed changes

address Yingyi Comments

f401d23

jsjtzyy merged commit 607df93 into linkedin:master Jan 14, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor AsyncRequestResponseHandlerFactory #1356

Refactor AsyncRequestResponseHandlerFactory #1356

zzmao commented Jan 10, 2020

codecov-io commented Jan 10, 2020 •

edited

Loading

cgtz Jan 11, 2020

cgtz Jan 11, 2020

zzmao Jan 11, 2020

cgtz Jan 11, 2020

cgtz Jan 11, 2020

cgtz commented Jan 11, 2020

zzmao commented Jan 11, 2020

jsjtzyy Jan 13, 2020

jsjtzyy Jan 13, 2020

jsjtzyy left a comment

jsjtzyy Jan 13, 2020

jsjtzyy Jan 13, 2020

jsjtzyy Jan 13, 2020

jsjtzyy Jan 13, 2020

jsjtzyy Jan 13, 2020

zzmao Jan 14, 2020

jsjtzyy Jan 13, 2020

		@@ -233,7 +235,7 @@ public void start() throws InstantiationException {
		restResponseHandler.start();

Refactor AsyncRequestResponseHandlerFactory #1356

Refactor AsyncRequestResponseHandlerFactory #1356

Conversation

zzmao commented Jan 10, 2020

codecov-io commented Jan 10, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cgtz commented Jan 11, 2020

zzmao commented Jan 11, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsjtzyy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Jan 10, 2020 •

edited

Loading