Rename BlobStorageService to RestRequestService #1342

zzmao · 2019-12-17T21:58:39Z

Rename BlobStorageService to RestRequestService, for

RestRequestService makes more sense since the interface arguments are RestRequest.
Create have clearer names for FrontendRestRequestService and StorageServerRestRequestService.

---DO NOT COMMIT MESSAGE BELOW---
grep 'BlobStorage|blobStorage' -r . | grep -v 'Azure|Binary file|AzureCloud'
./docs/release-notes.md: - Making sure ordering of tests does not matter for BlobStorage tests (#773)
./docs/release-notes.md: - Rewriting AmbryBlobStorageService to make callbacks cleaner (#250)
./docs/release-notes.md: - Wiring in IdConverter and SecurityService into AmbryBlobStorageService (#248)
./docs/release-notes.md: - Making sure ordering of tests does not matter for BlobStorage tests (#773)
./docs/release-notes.md: - Rewriting AmbryBlobStorageService to make callbacks cleaner (#250)
./docs/release-notes.md: - Wiring in IdConverter and SecurityService into AmbryBlobStorageService (#248)
./ambry.iws: ambryBlobStorageService
./ambry.iws: AmbryBlobStorageService
./ambry.iws: BlobStorage
./ambry.iws:
./ambry.iws:
./ambry.iws:
./.git/logs/HEAD:a8014eb1ebac94cd6994337819c4973108d3ee1c 13f05671e7e37eac7af1d6847e6f0edc4404cdeb Ze Mao zemao@linkedin.com 1576612062 -0800 commit: Rename BlobStorageService to RestRequestService
./.git/logs/refs/heads/master:a8014eb1ebac94cd6994337819c4973108d3ee1c 13f05671e7e37eac7af1d6847e6f0edc4404cdeb Ze Mao zemao@linkedin.com 1576612062 -0800 commit: Rename BlobStorageService to RestRequestService
[zemao@zemao-mn1]:ambry(master) $ git log

cgtz

LGTM after these comments are addressed.

cgtz · 2019-12-17T22:06:19Z

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

- * and generates a response. The information received from the scaling layer should be enough to perform these
- * functions.
- * <p/>
+ * RestRequestService defines a service that handles {@RestRequest}.


{@link RestRequest}s

cgtz · 2019-12-17T22:24:56Z

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

@@ -40,12 +40,12 @@
  private boolean isRunning = false;
  private VerifiableProperties failureProperties = null;

-  private BlobStorageService blobStorageService = null;
+  private RestRequestService _restRequestService = null;


let's not use _ before member variables since its not used elsewhere in ambry open source.

cgtz · 2019-12-17T22:30:06Z

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

@@ -191,17 +191,17 @@ public FrontendMetrics(MetricRegistry metricRegistry) {
    // RestRequestMetricsGroup
    // DELETE
    deleteBlobMetricsGroup =
-        new RestRequestMetricsGroup(AmbryBlobStorageService.class, "DeleteBlob", false, true, metricRegistry);
+        new RestRequestMetricsGroup(FrontendRestRequestService.class, "DeleteBlob", false, true, metricRegistry);


Reminder that this changes a lot of metric names so setup in closed source wrapper also have to be adjusted.

cgtz · 2019-12-17T22:34:45Z

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

@@ -854,7 +854,7 @@ public void onCompletion(final GetBlobResult routerResult, Exception routerExcep
                        response = new ByteBufferReadableStreamChannel(ByteBuffer.wrap(blobInfo.getUserMetadata()));
                      } else {
                        restResponseChannel.setHeader(Headers.CONTENT_LENGTH, 0);
-                        response = new ByteBufferReadableStreamChannel(AmbryBlobStorageService.EMPTY_BUFFER);
+                        response = new ByteBufferReadableStreamChannel(FrontendRestRequestService.EMPTY_BUFFER);


Unrelated to this PR, but could you change this to just be EMPTY_BUFFER since its in the same class.

cgtz · 2019-12-17T22:36:23Z

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

@@ -54,7 +54,7 @@
  private final Logger logger = LoggerFactory.getLogger(getClass());

  private AsyncResponseHandler asyncResponseHandler = null;
-  private BlobStorageService blobStorageService = null;
+  private RestRequestService _restRequestService = null;


remove underscore

cgtz · 2019-12-17T22:36:57Z

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

@@ -72,7 +72,7 @@
  private final JmxReporter reporter;
  private final AccountService accountService;
  private final Router router;
-  private final BlobStorageService blobStorageService;
+  private final RestRequestService _restRequestService;


remove underscore

cgtz · 2019-12-18T00:49:07Z

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

@@ -73,7 +73,7 @@
 */
 public class NettyMessageProcessorTest {
  private final InMemoryRouter router;
-  private final BlobStorageService blobStorageService;
+  private final RestRequestService _restRequestService;


remove underscore

cgtz · 2019-12-18T00:49:48Z

ambry-server/src/main/java/com.github.ambry.server/StorageServerRestRequestService.java

 * All the operations that need to be performed by the Ambry storage server are supported here.
 */
-public class ServerBlobStorageService implements BlobStorageService {
-  private static final Logger logger = LoggerFactory.getLogger(ServerBlobStorageService.class);
+public class StorageServerRestRequestService implements RestRequestService {


How about just StorageRestRequestService

codecov-io · 2019-12-18T18:48:37Z

Codecov Report

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

@@            Coverage Diff            @@
##             master    #1342   +/-   ##
=========================================
  Coverage          ?   72.06%           
  Complexity        ?     6527           
=========================================
  Files             ?      478           
  Lines             ?    37349           
  Branches          ?     4718           
=========================================
  Hits              ?    26917           
  Misses            ?     9172           
  Partials          ?     1260

Impacted Files	Coverage Δ	Complexity Δ
....github.ambry/account/ServiceIdAccountGenTool.java	`0% <ø> (ø)`	`0 <0> (?)`
...main/java/com.github.ambry/config/NettyConfig.java	`100% <ø> (ø)`	`1 <0> (?)`
...src/main/java/com.github.ambry/rest/RestUtils.java	`93.92% <ø> (ø)`	`68 <0> (?)`
...ub.ambry.rest/FrontendNettyChannelInitializer.java	`100% <ø> (ø)`	`3 <0> (?)`
...n/java/com.github.ambry/config/FrontendConfig.java	`100% <ø> (ø)`	`3 <0> (?)`
...m.github.ambry/rest/RestRequestMetricsTracker.java	`96.29% <ø> (ø)`	`15 <0> (?)`
...a/com.github.ambry.rest/NettyMessageProcessor.java	`78.82% <ø> (ø)`	`41 <0> (?)`
...github.ambry.server/StorageRestRequestService.java	`0% <0%> (ø)`	`0 <0> (?)`
...ub.ambry.frontend/AccountAndContainerInjector.java	`90.72% <0%> (ø)`	`27 <0> (?)`
...rc/main/java/com.github.ambry.rest/RestServer.java	`91.87% <100%> (ø)`	`13 <0> (?)`
... and 6 more

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 53f7a2c...408ce40. Read the comment docs.

Rename BlobStorageService to RestRequestService

bd03aea

zzmao requested review from cgtz and justinlin-linkedin December 17, 2019 21:58

cgtz suggested changes Dec 18, 2019

View reviewed changes

address cgetz comments

408ce40

cgtz approved these changes Dec 18, 2019

View reviewed changes

justinlin-linkedin approved these changes Dec 18, 2019

View reviewed changes

justinlin-linkedin merged commit 37c6246 into linkedin:master Dec 18, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rename BlobStorageService to RestRequestService #1342

Rename BlobStorageService to RestRequestService #1342

zzmao commented Dec 17, 2019

cgtz left a comment

cgtz Dec 17, 2019

cgtz Dec 17, 2019

cgtz Dec 17, 2019

zzmao Dec 18, 2019

cgtz Dec 17, 2019

cgtz Dec 17, 2019

cgtz Dec 17, 2019

cgtz Dec 18, 2019

cgtz Dec 18, 2019

codecov-io commented Dec 18, 2019

Rename BlobStorageService to RestRequestService #1342

Rename BlobStorageService to RestRequestService #1342

Conversation

zzmao commented Dec 17, 2019

cgtz 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Dec 18, 2019

Codecov Report