PutManager initial implementation #255

pnarayanan · 2016-04-06T03:23:41Z

This PR contains the core Put Manager implementation. A few other components that the Put Manager depends on, and certain flows within the NonBlocking router as a whole had to be modified/refactored in the process. However, the core implementation is limited to three main classes:

PutManager: that manages all put operations,
PutOperation: created by the PutManager for every operation.
PutChunk/MetadataPutChunk: inner classes of a PutOperation, that do state maintenance for the chunks of an operation.

A quick look at the PutManagerTest will help get an idea of what the Put Manager supports and how it behaves.

Primary reviewers: Gopal, Ming, Siva

Production Code
To review, I would start with the core classes:

ambry-router/src/main/java/com.github.ambry.router/NonBlockingRouter.java
ambry-router/src/main/java/com.github.ambry.router/PutManager.java
ambry-router/src/main/java/com.github.ambry.router/PutOperation.java (PutOperation + PutChunk)

Most, if not all, of the changes made elsewhere in the production code should get covered as the changes above are reviewed.

Test Code
Helpers
In order to test the Put Manager (and the other operation managers going forward), the following are introduced:

MockServer: In-memory mock implementation of a server that exposes a send() method that takes a Send (request) and returns a BoundedByteBufferReceive (response). It also provides hooks to introduce server errors and to deterministically simulate intermittent errors (that helps in testing slipped puts and such).

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

MockSelector: Intercepts all the router requests (instead of them going to the real selector) and sends and receives requests and responses from MockServers. It also allows for setting bad states to cause failures during connect, send or poll.

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

MockServerLayout: A class that maintains a mapping from a DataNodeId in the MockClusterMap to a corresponding MockServer. Used by the MockSelector to get the MockServer to send a request to, given the host and port. This class is also used by the tests themselves to get a list of all the MockServers and the requests they received, in order to perform verification.

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

Actual tests

PutManager test: Main set of tests for the Put Manager testing various scenarios. These tests submit putBlob() operations and assert success or failure as appropriate. Tests ensure that all requests for the same chunk are identical, that the content of blobs created are identical to their original content, all the cleanup happen as expected when the router is closed, etc.

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

ChunkFillTest: This is simply a test that directly executes the chunk filling flow within PutOperation. This helped exercise and stabilize that flow before the whole operation was tested.

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

Coverage

Package                            Class, %           Method, %           Line, %
com.github.ambry.router            100% (29/ 29)      90.2% (147/ 163)    88.9% (727/ 818)

Class                              Class, %           Method, %           Line, %
ChunkState                         100% (1/ 1)        100% (2/ 2)         100% (5/ 5)
CoordinatorBackedRouter            100% (1/ 1)        100% (13/ 13)       90.7% (78/ 86)
CoordinatorBackedRouterFactory     100% (1/ 1)        100% (2/ 2)         100% (16/ 16)
CoordinatorBackedRouterMetrics     100% (1/ 1)        100% (1/ 1)         100% (25/ 25)
CoordinatorOperation               100% (2/ 2)        100% (7/ 7)         93.8% (76/ 81)
CoordinatorOperationType           100% (1/ 1)        100% (2/ 2)         100% (5/ 5)
DeleteManager                      100% (1/ 1)        50% (1/ 2)          50% (1/ 2)
FutureResult                       100% (1/ 1)        69.2% (9/ 13)       68.2% (15/ 22)
GetManager                         100% (1/ 1)        100% (1/ 1)         100% (1/ 1)
NonBlockingRouter                  100% (3/ 3)        69% (20/ 29)        69.3% (88/ 127)
NonBlockingRouterFactory           100% (1/ 1)        100% (3/ 3)         85% (17/ 20)
NonBlockingRouterMetrics           100% (1/ 1)        100% (1/ 1)         100% (2/ 2)
PutManager                         100% (3/ 3)        100% (13/ 13)       95.9% (70/ 73)
PutOperation                       100% (5/ 5)        100% (47/ 47)       94% (220/ 234)
ReadableStreamChannelInputStream   100% (2/ 2)        87.5% (7/ 8)        90.6% (48/ 53)
RouterErrorCode                    100% (1/ 1)        100% (3/ 3)         87.5% (14/ 16)
RouterException                    100% (1/ 1)        80% (4/ 5)          76.9% (10/ 13)
SimpleOperationTracker             100% (2/ 2)        100% (11/ 11)       97.3% (36/ 37)

… testing

nsivabalan · 2016-04-06T17:03:58Z

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

+    return isOpen.get();
+  }
+
+  /**
   * Close the PutManager.
   */
  void close() {


If you feel we should have close, shouldn't all the managers implement Closeable?

Question about line 199. We wait on chunkFillerThread to join here. But we set the open state to false on exiting (finally block) the chunkFillerThread. So, does it make any difference. I feel like its more like a no-op.

got it. thanks

PutManager is special because it has a thread running in it. But we are being consistent across the operation managers.
I don't think there is a particular value in extending Closeable as these are not public classes.

nsivabalan · 2016-04-06T17:29:27Z

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

+    while (TestUtils.numThreadsByThisName("RequestResponseHandlerThread") > 0) {
+      Thread.yield();
+    }
+


Similar to above test, you could try to put a blob here and ensure all 3 operations are failed.

I think that is unnecessary here. If the RequestResponseHandler thread dies, it will close all operations. For the other test where the ChunkFiller thread dies, we close operations only when a new request comes in and realizes it (which was done to keep things simple).

xiahome · 2016-04-07T02:30:05Z

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

-  private static final AtomicLong operationIdGenerator = new AtomicLong(0);
+  private static final AtomicInteger currentOperationsCount = new AtomicInteger(0);
+
+  static final int SHUTDOWN_WAIT_MS = 10 * Time.MsPerSec;


Should this be 10 instead of 10 * Time.MsPerSec?

No, it is meant to be 10 seconds.

nsivabalan · 2016-04-07T05:45:03Z

Looks good to me.

xiahome · 2016-04-07T05:56:30Z

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

+      } catch (Exception e) {
+        logger.error("Aborting, as ChunkFillerThread received an exception: ", e);
+      } finally {
+        isOpen.set(false);


@pnarayanan Please correct me if I was wrong. If there is any exception during ChunkFiller thread, it will finally set PutManager's isOpen to false. So, the PutManager is closed.

Then, what is the behavior of the rest of the world? The router will not be closed, right? There can be no more PutOperations submitted to the PutManager, because it will check if the PutManager is closed. But how about the pending PutOperations? Will they be timed out? Also, it seems the OperationController is still able to handle responses for PutOperation in a normal way.

As Ming is taking off for the rest of the week, I am gonna handle this comment. He just has this one comment. Once this is addressed, we are good to merge.

If the ChunkFillerThread fails, we do not immediately close the router. The close logic is already a bit complicated and trying to do that would over complicate things. We are keeping it simple and making sure that if the ChunkFillerThread dies, it will simply close the PutManager.

The router will get closed when the next put operation gets submitted, and all existing operations will be disposed off. I think that is good enough, unless we see an issue in the future.

As an aside, note that in other parts of our code, we do not generally attempt to close everything down when a thread dies.

I get what you are trying to say. But I would really like to revisit this, but lets not hold this patch anymore. So, can you add this also to your follow up list so that we can discuss later.

xiahome · 2016-04-07T06:11:05Z

Left one comment, the rest LGTM.

nsivabalan · 2016-04-07T21:42:59Z

Looks good. Have you applied coding style and has the build passed successfully after all the changes?

pnarayanan · 2016-04-07T22:24:49Z

The changes build fine and the coding style is taken care of. There are intermittent errors that we have seen from time to time in the server tests, but those are unrelated to this patch.

Could you choose the squash and merge option when we merge. Here are the steps:

Click on "Merge pull request"
In the "Confirm Merge" button, click on the dropdown and select "Squash and Merge"
You will see each of the commit's title in the message. Remove everything and just keep "PutManager initial implementation".
Click on "Confirm squash and merge".

nsivabalan · 2016-04-07T22:27:40Z

Cool. Merging now. I mean, squashing and merging.

pnarayanan added 19 commits March 14, 2016 02:18

PutManager initial implementation

0a3e451

Tighten router class abstractions

5f4bab4

Incorporate review comments

b41a302

Incorporate test related comments

991533b

Removing a log file that crept in

fbc7e3a

Cleaning up todos

f2d4929

Addressing review comments

1b82a9f

minor refactor

f7f0156

PutManager initial implementation

2638697

Tighten router class abstractions

c6320c8

Incorporate review comments

094dc7b

Incorporate test related comments

7ff75c1

Removing a log file that crept in

35200d0

Cleaning up todos

a67d111

Addressing review comments

748e808

minor refactor

5bdf3f1

Fix ChunkFiller thread exit flow; add more tests for thread exit flow…

82224f3

… testing

Update copyright notice

db0f19f

Fix merge conflicts

124d206

pnarayanan mentioned this pull request Apr 6, 2016

PutManager initial implementation #236

Closed

nsivabalan reviewed Apr 6, 2016
View reviewed changes

Fix a minor merge conflict

a22d06a

nsivabalan reviewed Apr 6, 2016
View reviewed changes

minor MockSelector change

311ef8a

xiahome reviewed Apr 7, 2016
View reviewed changes

pnarayanan added 2 commits April 7, 2016 14:11

Refactor the close logic a little

b349e50

Rename doClose()

d417070

nsivabalan merged commit ac1cfbf into linkedin:master Apr 7, 2016

pnarayanan mentioned this pull request May 13, 2016

Implement support for getBlob() in the router #296

Closed

pnarayanan deleted the PutImplementationBak branch May 17, 2016 19:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PutManager initial implementation #255

PutManager initial implementation #255

pnarayanan commented Apr 6, 2016

nsivabalan Apr 6, 2016

nsivabalan Apr 6, 2016

nsivabalan Apr 6, 2016

pnarayanan Apr 6, 2016

nsivabalan Apr 6, 2016

pnarayanan Apr 6, 2016

xiahome Apr 7, 2016

pnarayanan Apr 7, 2016

nsivabalan commented Apr 7, 2016

xiahome Apr 7, 2016

nsivabalan Apr 7, 2016

pnarayanan Apr 7, 2016

nsivabalan Apr 7, 2016

xiahome commented Apr 7, 2016

nsivabalan commented Apr 7, 2016

pnarayanan commented Apr 7, 2016

nsivabalan commented Apr 7, 2016

PutManager initial implementation #255

PutManager initial implementation #255

Conversation

pnarayanan commented Apr 6, 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

nsivabalan commented Apr 7, 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

xiahome commented Apr 7, 2016

nsivabalan commented Apr 7, 2016

pnarayanan commented Apr 7, 2016

nsivabalan commented Apr 7, 2016