Jean-Daniel Cryans has posted comments on this change. Change subject: [java client] Implement RPC tracing, part 1 ......................................................................
Patch Set 3: (4 comments) http://gerrit.cloudera.org:8080/#/c/4781/3//COMMIT_MSG Commit Message: Line 30: RpcTraceObject{rpcMethod='Write', timestampMs=1477079973579, action=SEND_TO_SERVER, server=0353a6d97d6c49f9a727bc1ee6c3393e}, > Hmm, how is that possible? Did you ctrl-c the program or something? Or is i It's just an incomplete trace that I grabbed from the log. 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) > But we emit PICKED_REPLICA right before a master RPC too (e.g. GetTableLoca Yeah I was thinking of adding an optional "payload" to the trace object and this is one instance where it could be useful. What are your thoughts on this? Line 1001: rpc.setTimeoutMillis(parentRpc.deadlineTracker.getMillisBeforeDeadline()); > Alright, then please update the commit message to reflect these changes. Done 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 78: private KuduRpc<?> parentRpc; > I disagree pretty strongly with your position. Wherever we use RPC, at least in the Java client, we could probably use "Operation" instead, although that term is already used to describe what sessions apply. Overall I'm not sure we're actually in disagreement. Reading through the links you provided above, what I'm proposing with this patch fulfills the need you state. If you look at the trace I posted in the commit message (imagine it ends with RECEIVE_FROM_SERVER for the sake of this discussion), it seems all we're missing is a first "Flushing batch" trace (and as a payload we could add which tablet it is we're flushing the batch to). Specifically to your answer to #4, the user would know the trace is associated to the Flush because it'd come with the exception that that flush is sending back (not implemented in this patch). So I didn't previously add the flush trace because IMO it's obvious the trace is coming from the flush that's error'ing on you. -- 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 <jdcry...@apache.org> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Jean-Daniel Cryans <jdcry...@apache.org> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-HasComments: Yes
