Added DeleteManager and DeleteOperation. #239

xiahome · 2016-03-16T05:11:31Z

Patch for DeleteManager and DeleteOperation:

Some improvements and fixes for DeleteManager

merged the PutManagerpatch into DeleteManager patch.
added more test cases for completeness.
made delete operation be complied with operation tracker. If operation track decides the operation has been completed, no need to wait for more responses (especially for Blob_Not_Found).
added to SimpleOperationTracker to disable shuffling replicas to make deterministic processing order for replica responses.
modified part of code in PutManager for testing purpose (MockServerLayout and MockServer)
resolved conflicts between Put and Delete managers
solved router close issue in test (exception causes router to close during operation)
complied with any changes made in the NonBlockingRouter
improved documentation

Test coverage:

DeleteManager: 97.6%
DeleteOperation: 86.8% (uncovered part is for some not-like-to-happen cases. We have the code just for confidence and robustness)
RouterUtils: 91.7%

Test done: ./gradlew build

Reviewer: Priyesh, Siva

1. Added DeleteManager and DeleteOperation for non-blocking router; 2. Added Unit tests for these. 3. Modified MockServer for the test. Will later resolve Put patch and resolve conflicts. 4. The code largely follows PutManager and PutOperation, please refer to those for review. 5. This is an initial patch subject to further modifications and change merging with Put. To save your time please check at high level. 6. Log and metrics not included yet. Will work them out together with Put part. Test done: ./gradlew build Test coverage: DeleteManager: 84.2% (due to close() method not tested), DeleteOperation: 89.8% Reviewer: Priyesh, Siva, Gopal

vgkholla · 2016-03-23T19:20:04Z

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

+    routerPutRequestParallelism = verifiableProperties.getInt("router.put.request.parallelism", 3);
+    routerPutSuccessTarget = verifiableProperties.getInt("router.put.success.target", 2);
+    // @todo Verify the numbers here. In coordinator the policy for delete operation is to issue requests to
+    // @todo all replicas. This also need to be hard coded to 12 if lsg is counted.


please don't write any comments that contain internal details.

As for the todo, we can always configure accordingly no matter what the default is.

Thanks for this suggestion. Will remove all internal details.

is this addressed ?

Now addressed, thanks this was missed in the previous commit.

Tried to largely follow some logic as used in Put (after the new patch). I do see there is common parts between Put and Delete, so will take offline discussion to see if it is reasonable to make some abstract classes. Test coverage: DeleteManager: 97.6%, DeleteOperation: 90.2% (due to rare exceptions), RouterUtils: 91.7%.

nsivabalan · 2016-04-04T22:24:07Z

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

+    this.routerConfig = routerConfig;
+    this.routerMetrics = routerMetrics;
+    this.time = time;
+    deleteOperations = new HashSet<DeleteOperation>();


Why not concurrentHashSet as we have in PutManager? both Put qps and delete qps are pretty similar. So, why not follow same in both

The reason for using a HashSet is that, this set will only be accessed by the OperationController thread that owns the DeleteManager. However it will make sense to use a ConcurrentHashSet for two reasons: 1) be consistent with the PutManager, 2) for our confidence in synchronization issue. Let me know if you agree on this.

This will have to be a concurrent hash set (which is why it is one in PutManager). Entries get added to this set concurrently and in the context of the calling threads of the router library (not the OperationController). The flow would be router.deleteBlob() -> deleteManager.submitDeleteOperation() -> deleteOperations.add()

@pnarayanan makes sense, thank you. I will make the change.

nsivabalan · 2016-04-28T22:28:52Z

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

+    mockSelectorState = new AtomicReference<MockSelectorState>(MockSelectorState.Good);
+    clusterMap = new MockClusterMap();
+    serverLayout = new MockServerLayout(clusterMap);
+    setServerResponse(true);


why can't we set this value to true in constructor(of MockServer) and not set it everytime. We should set it only for exceptional cases i.e for failure cases

There are two tests that need this specifically set false.

nsivabalan · 2016-04-28T22:42:09Z

Done with my pass

pnarayanan · 2016-04-29T02:00:03Z

Looks good. Siva, once you finish, let me know (or go ahead and merge using the squash and merge option and editing the commit log to avoid irrelevant commit messages). Let us push this in by tomorrow.

nsivabalan · 2016-04-29T06:22:09Z

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

+      assertEquals("RouterErrorCode should be BlobDoesNotExist", expectedRouterErrorCode,
+          ((RouterException) e.getCause()).getErrorCode());
+    }
+  }
 }


@nsivabalan Do you mean to add a new line?

nsivabalan · 2016-04-29T06:37:26Z

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

@@ -170,27 +153,30 @@ public void testBlobExpired()
  }

  /**
-   * Test the case when the blob cannot be found in store servers.
+   * Test if the {@link RouterErrorCode} is as expected for differe {@link ServerErrorCode}.


typo "different"

nsivabalan · 2016-04-29T17:11:07Z

Looks good. Have you applied code styling? and build succeeded? Also, can you squash all the commits.

vgkholla · 2016-04-29T17:12:40Z

@nsivabalan I thought we were squashing when merging now?

nsivabalan · 2016-04-29T17:15:37Z

Its causing the commit history to differ in our repo and linkedin master. We can get away with it with some additional steps. But I felt if we can squash by ourselves, it solves the problem easily.

pnarayanan · 2016-04-29T17:19:53Z

Let us not squash ourselves. Could we hold on? The commit history should not differ if we abandon the branch used for the pull request.

vgkholla · 2016-04-29T17:20:35Z

We should take this offline but as I emailed the other day, you shouldn't be working on your master. @xiahome has this on a branch - his commit history will be fine. We should all be working on branches.

The second reason for squash and merge: Even if we squash manually, if we don't use the squash and merge option there will be a commit in the main repository's commit history that says "Merged by X", which we don't want either. We have to use the squash and merge option.

nsivabalan · 2016-04-29T17:22:14Z

ok, got you. My bad. I forgot. So, can I go ahead with the merge then ?

vgkholla · 2016-04-29T17:23:33Z

Once he confirms the formatting and build :)

pnarayanan · 2016-04-29T17:41:52Z

Also make sure to remove irrelevant lines from the commit message when you do the squash and merge. You would usually need just the original commit's message, though it can happen that certain subsequent commits were for added features.

pnarayanan · 2016-04-29T17:45:27Z

@xiahome Do you add the whole PR information into the commit message? Let us avoid that and keep the message short. The message anyway has a reference to the PR number.

@nsivabalan when you squash and merge, I think you only need to use the first message for this one.

vgkholla · 2016-04-29T17:54:05Z

+1 I find commit messages most useful when they are of this format

EDIT: Some better articles on writing good commit messages
https://github.com/erlang/otp/wiki/Writing-good-commit-messages
http://chris.beams.io/posts/git-commit/

They recommend the long description to completely explain the change and the short description to be < 50 chars.

nsivabalan · 2016-04-29T18:11:59Z

Fine. I am going ahead with the merge. Synced up with Ming offline wrt build and styling

vgkholla reviewed Mar 23, 2016
View reviewed changes

xiahome force-pushed the deleteManagerPatch branch from bfa6e20 to 66159f2 Compare March 31, 2016 18:37

nsivabalan reviewed Apr 4, 2016
View reviewed changes

nsivabalan reviewed Apr 28, 2016
View reviewed changes

xiahome force-pushed the deleteManagerPatch branch from 9bfb558 to b4626a0 Compare April 29, 2016 06:10

nsivabalan reviewed Apr 29, 2016
View reviewed changes

xiahome force-pushed the deleteManagerPatch branch 2 times, most recently from 916e5a2 to 8fddd35 Compare April 29, 2016 06:33

nsivabalan reviewed Apr 29, 2016
View reviewed changes

Addressed comments from Siva.

18e6472

xiahome force-pushed the deleteManagerPatch branch from 8fddd35 to 18e6472 Compare April 29, 2016 06:43

nsivabalan merged commit 6f18cae into linkedin:master Apr 29, 2016

pnarayanan mentioned this pull request May 13, 2016

Implement support for getBlob() in the router #296

Closed

xiahome deleted the deleteManagerPatch branch May 25, 2016 22:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added DeleteManager and DeleteOperation. #239

Added DeleteManager and DeleteOperation. #239

xiahome commented Mar 16, 2016

vgkholla Mar 23, 2016

xiahome Mar 28, 2016

nsivabalan Apr 4, 2016

xiahome Apr 6, 2016

nsivabalan Apr 4, 2016

xiahome Apr 5, 2016

pnarayanan Apr 5, 2016

xiahome Apr 5, 2016

nsivabalan Apr 28, 2016

xiahome Apr 29, 2016

xiahome Apr 29, 2016

nsivabalan commented Apr 28, 2016

pnarayanan commented Apr 29, 2016

nsivabalan Apr 29, 2016

xiahome Apr 29, 2016

nsivabalan Apr 29, 2016

nsivabalan commented Apr 29, 2016

vgkholla commented Apr 29, 2016

nsivabalan commented Apr 29, 2016

pnarayanan commented Apr 29, 2016

vgkholla commented Apr 29, 2016

nsivabalan commented Apr 29, 2016

vgkholla commented Apr 29, 2016

pnarayanan commented Apr 29, 2016

pnarayanan commented Apr 29, 2016

vgkholla commented Apr 29, 2016 •

edited

Loading

nsivabalan commented Apr 29, 2016

Added DeleteManager and DeleteOperation. #239

Added DeleteManager and DeleteOperation. #239

Conversation

xiahome commented Mar 16, 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

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

nsivabalan commented Apr 28, 2016

pnarayanan commented Apr 29, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nsivabalan commented Apr 29, 2016

vgkholla commented Apr 29, 2016

nsivabalan commented Apr 29, 2016

pnarayanan commented Apr 29, 2016

vgkholla commented Apr 29, 2016

nsivabalan commented Apr 29, 2016

vgkholla commented Apr 29, 2016

pnarayanan commented Apr 29, 2016

pnarayanan commented Apr 29, 2016

vgkholla commented Apr 29, 2016 • edited Loading

nsivabalan commented Apr 29, 2016

vgkholla commented Apr 29, 2016 •

edited

Loading