You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Jean-Daniel Cryans (Code Review)" <ge...@cloudera.org> on 2016/11/04 16:49:51 UTC
[kudu-CR] [java client] Implement RPC tracing, part 2
Hello Adar Dembo, Alexey Serbin,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/4950
to review the following change.
Change subject: [java client] Implement RPC tracing, part 2
......................................................................
[java client] Implement RPC tracing, part 2
This patch adds pretty printing for the traces, and makes them always part
of KuduRPC.toString. We thus rely on having the exceptions' messages to have
the RPC's string representation in them in order to have the traces make their
way back to the user.
In the future we could entertain having the traces in some queryable fashion in
KuduException, either be embedding the KuduRpc or just the list of traces.
Here's a trace snippet where a tserver that had a leader tablet was killed and we're
spinning on finding the new leader:
[0ms] sending RPC to server 5df1a37bd7e14380b5a8fceeba6b0d07
[70ms] received from server 5df1a37bd7e14380b5a8fceeba6b0d07 response Network error: [Peer 5df1a37bd7e14380b5a8fceeba6b0d07] Connection reset
[70ms] delaying RPC due to Network error: [Peer 5df1a37bd7e14380b5a8fceeba6b0d07] Connection reset
[101ms] querying master, [101ms] Sub rpc: GetTableLocations sending RPC to server e6e499debd804a8183b85d20a33ad560
[104ms] Sub rpc: GetTableLocations sending RPC to server e6e499debd804a8183b85d20a33ad560
[104ms] Sub rpc: GetTableLocations received from server e6e499debd804a8183b85d20a33ad560 response OK
[109ms] sending RPC to server 5df1a37bd7e14380b5a8fceeba6b0d07
[110ms] received from server 5df1a37bd7e14380b5a8fceeba6b0d07 response Network error: [Peer 5df1a37bd7e14380b5a8fceeba6b0d07] Connection reset
Change-Id: I2c37143a587971f0e8985c59f4ab1d0c164c3723
---
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduRpc.java
M java/kudu-client/src/main/java/org/apache/kudu/client/RpcTraceObject.java
2 files changed, 59 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/50/4950/1
--
To view, visit http://gerrit.cloudera.org:8080/4950
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I2c37143a587971f0e8985c59f4ab1d0c164c3723
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
[kudu-CR] [java client] Implement RPC tracing, part 2
Posted by "Jean-Daniel Cryans (Code Review)" <ge...@cloudera.org>.
Jean-Daniel Cryans has posted comments on this change.
Change subject: [java client] Implement RPC tracing, part 2
......................................................................
Patch Set 1:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/4950/1//COMMIT_MSG
Commit Message:
PS1, Line 10: We thus rely on having the exceptions' messages to have
: the RPC's string representation in them in order to have the traces make their
: way back to the user.
> Nit: a little clunky ("have" or "having" three times), reword?
Done
PS1, Line 15: be embedding the KuduRpc or just the list of traces.
> Nit: "by embedding the KuduRpc or just its list of traces"
Done
Line 23: [101ms] querying master, [101ms] Sub rpc: GetTableLocations sending RPC to server e6e499debd804a8183b85d20a33ad560
> Why were these two entries ("querying master" and "Sub rpc: ...") on the sa
Because I didn't break up the lines correctly.
http://gerrit.cloudera.org:8080/#/c/4950/1/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 104: case SEND_TO_SERVER:
> You can encapsulate these in AppendToStringBuilder(RpcTraceObject, StringBu
I'll call it appendToStringBuilder if you don't mind.
--
To view, visit http://gerrit.cloudera.org:8080/4950
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I2c37143a587971f0e8985c59f4ab1d0c164c3723
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes
[kudu-CR] [java client] Implement RPC tracing, part 2
Posted by "Jean-Daniel Cryans (Code Review)" <ge...@cloudera.org>.
Hello Adar Dembo, Alexey Serbin, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/4950
to look at the new patch set (#3).
Change subject: [java client] Implement RPC tracing, part 2
......................................................................
[java client] Implement RPC tracing, part 2
This patch adds pretty printing for the traces, and makes them always part
of KuduRPC.toString. We thus rely on the exceptions' messages to include
the RPC's string representation in order to propagate the traces back to
the user.
In the future we could entertain having the traces in some queryable fashion in
KuduException, either by embedding the KuduRpc or just the list of traces.
Here's a trace snippet where a tserver that had a leader tablet was killed and we're
spinning on finding the new leader:
[0ms] sending RPC to server 5df1a37bd7e14380b5a8fceeba6b0d07
[70ms] received from server 5df1a37bd7e14380b5a8fceeba6b0d07 response Network error: [Peer 5df1a37bd7e14380b5a8fceeba6b0d07] Connection reset
[70ms] delaying RPC due to Network error: [Peer 5df1a37bd7e14380b5a8fceeba6b0d07] Connection reset
[101ms] querying master
[101ms] Sub rpc: GetTableLocations sending RPC to server e6e499debd804a8183b85d20a33ad560
[104ms] Sub rpc: GetTableLocations sending RPC to server e6e499debd804a8183b85d20a33ad560
[104ms] Sub rpc: GetTableLocations received from server e6e499debd804a8183b85d20a33ad560 response OK
[109ms] sending RPC to server 5df1a37bd7e14380b5a8fceeba6b0d07
[110ms] received from server 5df1a37bd7e14380b5a8fceeba6b0d07 response Network error: [Peer 5df1a37bd7e14380b5a8fceeba6b0d07] Connection reset
Change-Id: I2c37143a587971f0e8985c59f4ab1d0c164c3723
---
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduRpc.java
M java/kudu-client/src/main/java/org/apache/kudu/client/RpcTraceFrame.java
2 files changed, 68 insertions(+), 6 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/50/4950/3
--
To view, visit http://gerrit.cloudera.org:8080/4950
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2c37143a587971f0e8985c59f4ab1d0c164c3723
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] [java client] Implement RPC tracing, part 2
Posted by "Jean-Daniel Cryans (Code Review)" <ge...@cloudera.org>.
Hello Adar Dembo, Alexey Serbin, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/4950
to look at the new patch set (#2).
Change subject: [java client] Implement RPC tracing, part 2
......................................................................
[java client] Implement RPC tracing, part 2
This patch adds pretty printing for the traces, and makes them always part
of KuduRPC.toString. We thus rely on the exceptions' messages to include
the RPC's string representation in order to propagate the traces back to
the user.
In the future we could entertain having the traces in some queryable fashion in
KuduException, either by embedding the KuduRpc or just the list of traces.
Here's a trace snippet where a tserver that had a leader tablet was killed and we're
spinning on finding the new leader:
[0ms] sending RPC to server 5df1a37bd7e14380b5a8fceeba6b0d07
[70ms] received from server 5df1a37bd7e14380b5a8fceeba6b0d07 response Network error: [Peer 5df1a37bd7e14380b5a8fceeba6b0d07] Connection reset
[70ms] delaying RPC due to Network error: [Peer 5df1a37bd7e14380b5a8fceeba6b0d07] Connection reset
[101ms] querying master
[101ms] Sub rpc: GetTableLocations sending RPC to server e6e499debd804a8183b85d20a33ad560
[104ms] Sub rpc: GetTableLocations sending RPC to server e6e499debd804a8183b85d20a33ad560
[104ms] Sub rpc: GetTableLocations received from server e6e499debd804a8183b85d20a33ad560 response OK
[109ms] sending RPC to server 5df1a37bd7e14380b5a8fceeba6b0d07
[110ms] received from server 5df1a37bd7e14380b5a8fceeba6b0d07 response Network error: [Peer 5df1a37bd7e14380b5a8fceeba6b0d07] Connection reset
Change-Id: I2c37143a587971f0e8985c59f4ab1d0c164c3723
---
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduRpc.java
M java/kudu-client/src/main/java/org/apache/kudu/client/RpcTraceObject.java
2 files changed, 68 insertions(+), 6 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/50/4950/2
--
To view, visit http://gerrit.cloudera.org:8080/4950
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2c37143a587971f0e8985c59f4ab1d0c164c3723
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] [java client] Implement RPC tracing, part 2
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: [java client] Implement RPC tracing, part 2
......................................................................
Patch Set 1:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/4950/1//COMMIT_MSG
Commit Message:
PS1, Line 10: We thus rely on having the exceptions' messages to have
: the RPC's string representation in them in order to have the traces make their
: way back to the user.
Nit: a little clunky ("have" or "having" three times), reword?
PS1, Line 15: be embedding the KuduRpc or just the list of traces.
Nit: "by embedding the KuduRpc or just its list of traces"
Line 23: [101ms] querying master, [101ms] Sub rpc: GetTableLocations sending RPC to server e6e499debd804a8183b85d20a33ad560
Why were these two entries ("querying master" and "Sub rpc: ...") on the same line?
http://gerrit.cloudera.org:8080/#/c/4950/1/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 104: case SEND_TO_SERVER:
You can encapsulate these in AppendToStringBuilder(RpcTraceObject, StringBuilder) methods, one for each Action enum.
--
To view, visit http://gerrit.cloudera.org:8080/4950
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I2c37143a587971f0e8985c59f4ab1d0c164c3723
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes
[kudu-CR] [java client] Implement RPC tracing, part 2
Posted by "Jean-Daniel Cryans (Code Review)" <ge...@cloudera.org>.
Jean-Daniel Cryans has submitted this change and it was merged.
Change subject: [java client] Implement RPC tracing, part 2
......................................................................
[java client] Implement RPC tracing, part 2
This patch adds pretty printing for the traces, and makes them always part
of KuduRPC.toString. We thus rely on the exceptions' messages to include
the RPC's string representation in order to propagate the traces back to
the user.
In the future we could entertain having the traces in some queryable fashion in
KuduException, either by embedding the KuduRpc or just the list of traces.
Here's a trace snippet where a tserver that had a leader tablet was killed and we're
spinning on finding the new leader:
[0ms] sending RPC to server 5df1a37bd7e14380b5a8fceeba6b0d07
[70ms] received from server 5df1a37bd7e14380b5a8fceeba6b0d07 response Network error: [Peer 5df1a37bd7e14380b5a8fceeba6b0d07] Connection reset
[70ms] delaying RPC due to Network error: [Peer 5df1a37bd7e14380b5a8fceeba6b0d07] Connection reset
[101ms] querying master
[101ms] Sub rpc: GetTableLocations sending RPC to server e6e499debd804a8183b85d20a33ad560
[104ms] Sub rpc: GetTableLocations sending RPC to server e6e499debd804a8183b85d20a33ad560
[104ms] Sub rpc: GetTableLocations received from server e6e499debd804a8183b85d20a33ad560 response OK
[109ms] sending RPC to server 5df1a37bd7e14380b5a8fceeba6b0d07
[110ms] received from server 5df1a37bd7e14380b5a8fceeba6b0d07 response Network error: [Peer 5df1a37bd7e14380b5a8fceeba6b0d07] Connection reset
Change-Id: I2c37143a587971f0e8985c59f4ab1d0c164c3723
Reviewed-on: http://gerrit.cloudera.org:8080/4950
Reviewed-by: Adar Dembo <ad...@cloudera.com>
Tested-by: Kudu Jenkins
---
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduRpc.java
M java/kudu-client/src/main/java/org/apache/kudu/client/RpcTraceFrame.java
2 files changed, 68 insertions(+), 6 deletions(-)
Approvals:
Adar Dembo: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/4950
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I2c37143a587971f0e8985c59f4ab1d0c164c3723
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] [java client] Implement RPC tracing, part 2
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: [java client] Implement RPC tracing, part 2
......................................................................
Patch Set 4: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/4950
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I2c37143a587971f0e8985c59f4ab1d0c164c3723
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No
[kudu-CR] [java client] Implement RPC tracing, part 2
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: [java client] Implement RPC tracing, part 2
......................................................................
Patch Set 3: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/4950
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I2c37143a587971f0e8985c59f4ab1d0c164c3723
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No