Adar Dembo has posted comments on this change. Change subject: [java client] Implement RPC tracing, part 1 ......................................................................
Patch Set 3: (16 comments) Would be nice to understand whether there's a perf impact to this, to decide whether we should add some kind of opt-in or opt-out switch at the client level. http://gerrit.cloudera.org:8080/#/c/4781/3//COMMIT_MSG Commit Message: PS3, Line 14: "parent RPC" As per the short discussion we had earlier with Todd, maybe we can call this something else? Like an "RPC flow", or "flow", or something like that? "Parent RPC" is a little weird because it implies that the top-level thing (or first thing) is an RPC, which it doesn't necessarily have to be. For example, I'd argue that a Flush() is the top-level thing users care about, which triggers one Write RPC per tserver. Line 30: RpcTraceObject{rpcMethod='Write', timestampMs=1477079973579, action=SEND_TO_SERVER, server=0353a6d97d6c49f9a727bc1ee6c3393e}, Nice. Shouldn't this end with a RECEIVE_FROM_SERVER callStatus=OK though? Or should there be a "..." at the end of this sample output? http://gerrit.cloudera.org:8080/#/c/4781/3/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java File java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java: Line 679: .action(RpcTraceObject.Action.PICKED_REPLICA) Why is this an interesting action? There's no real time spent doing it. Line 1001: rpc.setTimeoutMillis(parentRpc.deadlineTracker.getMillisBeforeDeadline()); These changes (to set a different timeout if there's a parent RPC) seem orthogonal to the tracing patch. Maybe you can split them out? Line 1191: Status reasonForRetry = ex == null ? null : ex.getStatus(); Seems like there should always be a reason to retry; under what circumstances is there no exception? Line 1463: shutdown().join(); This change should have been in your previous patch, I think. http://gerrit.cloudera.org:8080/#/c/4781/3/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java File java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java: Line 789: public String toString() { While you're here, could you convert this to use MoreObjects.toStringHelper()? http://gerrit.cloudera.org:8080/#/c/4781/3/java/kudu-client/src/main/java/org/apache/kudu/client/KuduRpc.java File java/kudu-client/src/main/java/org/apache/kudu/client/KuduRpc.java: Line 67: public static final int MAX_TRACES_SIZE = 100; Mark this as @VisibleForTesting Line 78: private KuduRpc<?> parentRpc; While a good first step, I don't think this is nearly descriptive enough. As I mentioned in a comment to the commit description, I think the "parent" object should be something that's not an RPC, but rather than something that directly maps to a user-triggered API call (e.g. Flush(), ScanNextRows()). In many cases that is an RPC, but for Flush() it's not, and Flush() is one of the most interesting cases. PS3, Line 216: traces.clear(); : parentRpc = null; Why is it important to reset this state here? Does it prevent GC'ing? Line 222: * Add the provided trace to this RPC's collection of traces. If this RPC has a parent RPC, it What does the parentRPC graph look like? Is it an arbitrary tree? Or is it simpler than that (i.e. maybe a tree of depth 2 at most)? http://gerrit.cloudera.org:8080/#/c/4781/3/java/kudu-client/src/main/java/org/apache/kudu/client/RpcTraceObject.java File java/kudu-client/src/main/java/org/apache/kudu/client/RpcTraceObject.java: Line 27: enum Action { Could you add a sentence or two description of each action? Line 76: (action == null ? "" : ", action=" + action) + How can action be null? http://gerrit.cloudera.org:8080/#/c/4781/3/java/kudu-client/src/test/java/org/apache/kudu/client/TestRpcTraces.java File java/kudu-client/src/test/java/org/apache/kudu/client/TestRpcTraces.java: PS3, Line 51: grandparent Shouldn't this be 'parent'? Both 'son' and 'daughter' imply the presence of a father/mother in between, and 'grandparent' seems like it's not that person. PS3, Line 62: daughter Shouldn't this be son? Line 75: assertEquals(1, sonsDaughter.getImmutableTraces().size()); This is unintuitive given what's being tested, but I'm guessing it's because you haven't reset the traces in between test cases. Could you do that? Then this should be 0, right? -- To view, visit http://gerrit.cloudera.org:8080/4781 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I69ef56acc071b9f80b34e38c1821df4096f54907 Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Jean-Daniel Cryans <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Jean-Daniel Cryans <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
