GetManager implementation with GetBlobInfo support #276

pnarayanan · 2016-05-02T19:41:21Z

This PR adds the initial GetManager implementation with support for getBlobInfo() operations, with associated unit tests.

This PR also includes a few fixes here and there within the PutManager and the mock classes, which are mostly minor. The most relevant of these changes is within the MockServer in the way it constructs the responses. With the changes, it reuses the functionalities from the production code rather than construct responses manually.

Because the changes for getBlob() support were part of this change and later removed from this PR in order to help simplify the reviews, there may be a few things that might seem unnecessary if you look at this patch in isolation (for example, one might wonder why certain methods are (or are not) in the abstract GetOperation class), so do keep in mind that those might be to support getBlob() (that will be put up as a PR once this gets merged).

Primary reviewers:
Siva, Gopal

Coverage

GetBlobInfoOperation    100% (1/ 1)    100% (6/ 6)    82.2% (60/ 73)
GetManager              100% (2/ 2)    100% (9/ 9)    92.7% (38/ 41)
GetOperation            100% (2/ 2)    100% (11/ 11)  97.1% (34/ 35)

vgkholla · 2016-05-02T21:15:41Z

ambry-api/src/main/java/com.github.ambry/messageformat/BlobProperties.java

@@ -131,4 +131,19 @@ public String toString() {
    sb.append("]");
    return sb.toString();
  }
+
+  @Override


If you write equals(), don't you need to write hashCode() too?

We haven't really been following that practice elsewhere in our codebase, but yes that is recommended practice. However, on second thought, I feel more comfortable not overriding equals() in the first place (MessageFormat actually returns the BlobProperties via a private SystemMetadata class that overrides BlobProperties and equals() below ignores the class equality). Will move this check into the test itself.

vgkholla · 2016-05-03T17:08:38Z

Done with production code. Going to take a look at test code late today or tomorrow.

nsivabalan · 2016-05-03T21:46:19Z

ambry-router/src/main/java/com.github.ambry.router/GetManager.java

+  // Because there is a guaranteed response from the NetworkClient for every request sent out, entries
+  // get cleaned up periodically.
+  private final Map<Integer, GetOperation> correlationIdToGetOperation = new HashMap<Integer, GetOperation>();
+  private final AtomicBoolean isOpen = new AtomicBoolean(true);


isOpen unused ?

nsivabalan · 2016-05-07T00:14:55Z

ambry-router/src/test/java/com.github.ambry.router/GetManagerTest.java

+  }
+
+  /**
+   * Tests the failure case where poll throws and closes the router. This also tests the case where the GetManager


Doc says, also tests the case where the GetManager is closed. Where exactly do you check that the getManager is closed?

tests the case where the GetManager gets closed, not that it is closed specifically. Basically, ensures that the GetManager#close() is called and completes existing operations with the RouterClosed error, by checking exactly that.

pnarayanan · 2016-05-07T01:26:57Z

Updated to address all review comments so far, please take a look.

nsivabalan · 2016-05-09T19:04:43Z

ambry-router/src/main/java/com.github.ambry.router/PutManager.java

-      NonBlockingRouter.completeOperation(op.getFuture(), op.getCallback(), null,
-          new RouterException("Aborted operation because Router is closed", RouterErrorCode.RouterClosed));
+    for (PutOperation op : putOperations) {
+      // There is a rare scenario where the operation gets removed from this set and gets completed concurrently by


How did you figure this out(duplicate completion). Some tests which does parallel puts ?

No - just from my understanding and analysis of the code. It is hard to reproduce this through any tests.

nsivabalan · 2016-05-09T21:03:23Z

Looks good

vgkholla · 2016-05-09T23:08:06Z

ambry-router/src/main/java/com.github.ambry.router/GetOperation.java

+
+  /**
+   * Set the exception associated with this operation.
+   * A {@link ServerErrorCode#Blob_Deleted} or {@link ServerErrorCode#Blob_Expired} error overrides any other


you might want to have a predictable order b/w expired and deleted blob to provide a consistent experience when the GET APIs are used.

Good point, but I think that is already being handled to the extent it can be. The chances of a blob being expired in some replicas and deleted in certain others is very slim, but even if that happens, here we ensure that the first such error that is reported is returned.

Agreed that the chances are slim, but for a short period of time, one call might return expired, and another deleted depending on a lot of factors.

It's not a big problem so we can add it to the backlog when we eventually have to handle precedence among all router error codes.

My point is that there is no way to handle this. It depends completely on the order in which replicas are contacted (which changes between calls). Not unless we continue to handle responses in case of Deleted/Expired.

And even if they are contacted in the same order, there is no guarantee on the order of the responses.

vgkholla · 2016-05-10T01:11:06Z

LGTM. Few minor comments - can merge once addressed

xiahome · 2016-05-10T05:32:35Z

ambry-router/src/main/java/com.github.ambry.router/GetOperation.java

+ * An abstract class for a get operation.
+ * @param <T> the type of the result of this operation.
+ */
+abstract class GetOperation<T> {


Maybe I missed your point. Why you make GetOperation abstract?

Two types of GetOperations - GetBlobInfoOperation and GetBlobOperation.

pnarayanan · 2016-05-10T06:26:08Z

Addressed all comments.

vgkholla · 2016-05-10T07:15:52Z

LGTM. Merging this but please follow up on any unresolved comments by @xiahome.

vgkholla · 2016-05-10T07:16:27Z

Also, I assume you have built and formatted.

xiahome · 2016-05-10T19:10:03Z

LGTM.

vgkholla reviewed May 2, 2016
View reviewed changes

nsivabalan reviewed May 3, 2016
View reviewed changes

nsivabalan reviewed May 7, 2016
View reviewed changes

Refactor, other fixes

3c097d5

Rename GetOperationTest to GetBlobInfoOperationTest

53f7590

nsivabalan reviewed May 9, 2016
View reviewed changes

fix a bug in GetManager#handleResponse()

03b4afd

vgkholla reviewed May 9, 2016
View reviewed changes

xiahome reviewed May 10, 2016
View reviewed changes

Test improvements

e499475

pnarayanan force-pushed the GetManagerAndBlobInfo branch from 97a7c94 to e499475 Compare May 10, 2016 06:32

vgkholla merged commit e6830a0 into linkedin:master May 10, 2016

pnarayanan mentioned this pull request May 13, 2016

Implement support for getBlob() in the router #296

Closed

pnarayanan deleted the GetManagerAndBlobInfo branch May 17, 2016 19:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GetManager implementation with GetBlobInfo support #276

GetManager implementation with GetBlobInfo support #276

pnarayanan commented May 2, 2016

vgkholla May 2, 2016

pnarayanan May 3, 2016

vgkholla commented May 3, 2016

nsivabalan May 3, 2016

pnarayanan May 5, 2016

nsivabalan May 7, 2016

pnarayanan May 7, 2016

pnarayanan commented May 7, 2016

nsivabalan May 9, 2016

pnarayanan May 9, 2016

nsivabalan commented May 9, 2016

vgkholla May 9, 2016

pnarayanan May 9, 2016

vgkholla May 9, 2016

pnarayanan May 9, 2016

pnarayanan May 9, 2016

vgkholla commented May 10, 2016

xiahome May 10, 2016

pnarayanan May 10, 2016

pnarayanan commented May 10, 2016

vgkholla commented May 10, 2016

vgkholla commented May 10, 2016

xiahome commented May 10, 2016

GetManager implementation with GetBlobInfo support #276

GetManager implementation with GetBlobInfo support #276

Conversation

pnarayanan commented May 2, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vgkholla commented May 3, 2016

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

pnarayanan commented May 7, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nsivabalan commented May 9, 2016

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

vgkholla commented May 10, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pnarayanan commented May 10, 2016

vgkholla commented May 10, 2016

vgkholla commented May 10, 2016

xiahome commented May 10, 2016