[kudu-CR] KUDU-2301: (Part-1) Add instrumentation on a per connection level
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/9343 ) Change subject: KUDU-2301: (Part-1) Add instrumentation on a per connection level .. Patch Set 9: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/9343 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iae1a5fe0066adf644a9cac41ad6696e1bbf00465 Gerrit-Change-Number: 9343 Gerrit-PatchSet: 9 Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Thu, 22 Feb 2018 18:06:20 + Gerrit-HasComments: No
[kudu-CR] KUDU-2301: (Part-1) Add instrumentation on a per connection level
Todd Lipcon has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/9343 ) Change subject: KUDU-2301: (Part-1) Add instrumentation on a per connection level .. KUDU-2301: (Part-1) Add instrumentation on a per connection level This patch returns the OutboundTransfer queue size on a per connection level and makes them accessible via the DumpRunningRpcs() call. A test is added in rpc-test to ensure that this metric works as expected. A future patch will add more metrics. Change-Id: Iae1a5fe0066adf644a9cac41ad6696e1bbf00465 Reviewed-on: http://gerrit.cloudera.org:8080/9343 Tested-by: Kudu Jenkins Reviewed-by: Todd Lipcon --- M src/kudu/rpc/connection.cc M src/kudu/rpc/connection.h M src/kudu/rpc/messenger.h M src/kudu/rpc/rpc-test.cc M src/kudu/rpc/rpc_introspection.proto 5 files changed, 68 insertions(+), 3 deletions(-) Approvals: Kudu Jenkins: Verified Todd Lipcon: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/9343 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: Iae1a5fe0066adf644a9cac41ad6696e1bbf00465 Gerrit-Change-Number: 9343 Gerrit-PatchSet: 10 Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-2301: (Part-1) Add instrumentation on a per connection level
Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/9343 ) Change subject: KUDU-2301: (Part-1) Add instrumentation on a per connection level .. Patch Set 9: (1 comment) http://gerrit.cloudera.org:8080/#/c/9343/8/src/kudu/rpc/rpc-test.cc File src/kudu/rpc/rpc-test.cc: http://gerrit.cloudera.org:8080/#/c/9343/8/src/kudu/rpc/rpc-test.cc@28 PS8, Line 28: #include > warning: #includes are not sorted properly [llvm-include-order] Done -- To view, visit http://gerrit.cloudera.org:8080/9343 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iae1a5fe0066adf644a9cac41ad6696e1bbf00465 Gerrit-Change-Number: 9343 Gerrit-PatchSet: 9 Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Tue, 20 Feb 2018 23:56:34 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2301: (Part-1) Add instrumentation on a per connection level
Hello Michael Ho, Tidy Bot, Lars Volker, Alexey Serbin, Dan Burkert, Kudu Jenkins, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/9343 to look at the new patch set (#9). Change subject: KUDU-2301: (Part-1) Add instrumentation on a per connection level .. KUDU-2301: (Part-1) Add instrumentation on a per connection level This patch returns the OutboundTransfer queue size on a per connection level and makes them accessible via the DumpRunningRpcs() call. A test is added in rpc-test to ensure that this metric works as expected. A future patch will add more metrics. Change-Id: Iae1a5fe0066adf644a9cac41ad6696e1bbf00465 --- M src/kudu/rpc/connection.cc M src/kudu/rpc/connection.h M src/kudu/rpc/messenger.h M src/kudu/rpc/rpc-test.cc M src/kudu/rpc/rpc_introspection.proto 5 files changed, 68 insertions(+), 3 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/43/9343/9 -- To view, visit http://gerrit.cloudera.org:8080/9343 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Iae1a5fe0066adf644a9cac41ad6696e1bbf00465 Gerrit-Change-Number: 9343 Gerrit-PatchSet: 9 Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-2301: (Part-1) Add instrumentation on a per connection level
Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/9343 ) Change subject: KUDU-2301: (Part-1) Add instrumentation on a per connection level .. Patch Set 8: (1 comment) http://gerrit.cloudera.org:8080/#/c/9343/7/src/kudu/rpc/rpc-test.cc File src/kudu/rpc/rpc-test.cc: http://gerrit.cloudera.org:8080/#/c/9343/7/src/kudu/rpc/rpc-test.cc@494 PS7, Line 494: ASSERT_OK(client_messenger->DumpRunningRpcs(dump_req, &dump_resp)); > Can you try looping this test with --stress-cpu-threads=$(nproc) to double I ran the test with --stress_cpu_threads=$(nrpoc) for about an hour (~800 iterations), and there were no failures. -- To view, visit http://gerrit.cloudera.org:8080/9343 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iae1a5fe0066adf644a9cac41ad6696e1bbf00465 Gerrit-Change-Number: 9343 Gerrit-PatchSet: 8 Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Tue, 20 Feb 2018 23:44:39 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2301: (Part-1) Add instrumentation on a per connection level
Hello Michael Ho, Lars Volker, Alexey Serbin, Dan Burkert, Kudu Jenkins, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/9343 to look at the new patch set (#8). Change subject: KUDU-2301: (Part-1) Add instrumentation on a per connection level .. KUDU-2301: (Part-1) Add instrumentation on a per connection level This patch returns the OutboundTransfer queue size on a per connection level and makes them accessible via the DumpRunningRpcs() call. A test is added in rpc-test to ensure that this metric works as expected. A future patch will add more metrics. Change-Id: Iae1a5fe0066adf644a9cac41ad6696e1bbf00465 --- M src/kudu/rpc/connection.cc M src/kudu/rpc/connection.h M src/kudu/rpc/messenger.h M src/kudu/rpc/rpc-test.cc M src/kudu/rpc/rpc_introspection.proto 5 files changed, 68 insertions(+), 3 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/43/9343/8 -- To view, visit http://gerrit.cloudera.org:8080/9343 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Iae1a5fe0066adf644a9cac41ad6696e1bbf00465 Gerrit-Change-Number: 9343 Gerrit-PatchSet: 8 Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-2301: (Part-1) Add instrumentation on a per connection level
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/9343 ) Change subject: KUDU-2301: (Part-1) Add instrumentation on a per connection level .. Patch Set 7: (1 comment) http://gerrit.cloudera.org:8080/#/c/9343/7/src/kudu/rpc/rpc-test.cc File src/kudu/rpc/rpc-test.cc: http://gerrit.cloudera.org:8080/#/c/9343/7/src/kudu/rpc/rpc-test.cc@494 PS7, Line 494: ASSERT_GT(dump_resp.outbound_connections(0).outbound_queue_size(), 0); Can you try looping this test with --stress-cpu-threads=$(nproc) to double check it isn't flaky? -- To view, visit http://gerrit.cloudera.org:8080/9343 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iae1a5fe0066adf644a9cac41ad6696e1bbf00465 Gerrit-Change-Number: 9343 Gerrit-PatchSet: 7 Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Tue, 20 Feb 2018 22:38:19 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2301: (Part-1) Add instrumentation on a per connection level
Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/9343 ) Change subject: KUDU-2301: (Part-1) Add instrumentation on a per connection level .. Patch Set 7: (1 comment) http://gerrit.cloudera.org:8080/#/c/9343/5/src/kudu/rpc/rpc-test.cc File src/kudu/rpc/rpc-test.cc: http://gerrit.cloudera.org:8080/#/c/9343/5/src/kudu/rpc/rpc-test.cc@489 PS5, Line 489: > I still don't think that helps, because the RPCs will still end up read off Ah right, that was a mistake. I changed it to keep the server's reactor thread busy by adding a sleep task for 2 seconds before queuing up RPCs. I tried using a latch, but the reactor thread restrictions hits a DCHECK, as we're not supposed to block on reactor threads. -- To view, visit http://gerrit.cloudera.org:8080/9343 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iae1a5fe0066adf644a9cac41ad6696e1bbf00465 Gerrit-Change-Number: 9343 Gerrit-PatchSet: 7 Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Tue, 20 Feb 2018 22:36:30 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2301: (Part-1) Add instrumentation on a per connection level
Hello Michael Ho, Lars Volker, Alexey Serbin, Dan Burkert, Kudu Jenkins, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/9343 to look at the new patch set (#7). Change subject: KUDU-2301: (Part-1) Add instrumentation on a per connection level .. KUDU-2301: (Part-1) Add instrumentation on a per connection level This patch returns the OutboundTransfer queue size on a per connection level and makes them accessible via the DumpRunningRpcs() call. A test is added in rpc-test to ensure that this metric works as expected. A future patch will add more metrics. Change-Id: Iae1a5fe0066adf644a9cac41ad6696e1bbf00465 --- M src/kudu/rpc/connection.cc M src/kudu/rpc/connection.h M src/kudu/rpc/messenger.h M src/kudu/rpc/rpc-test.cc M src/kudu/rpc/rpc_introspection.proto 5 files changed, 66 insertions(+), 3 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/43/9343/7 -- To view, visit http://gerrit.cloudera.org:8080/9343 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Iae1a5fe0066adf644a9cac41ad6696e1bbf00465 Gerrit-Change-Number: 9343 Gerrit-PatchSet: 7 Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-2301: (Part-1) Add instrumentation on a per connection level
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/9343 ) Change subject: KUDU-2301: (Part-1) Add instrumentation on a per connection level .. Patch Set 6: (1 comment) http://gerrit.cloudera.org:8080/#/c/9343/5/src/kudu/rpc/rpc-test.cc File src/kudu/rpc/rpc-test.cc: http://gerrit.cloudera.org:8080/#/c/9343/5/src/kudu/rpc/rpc-test.cc@489 PS5, Line 489: controllers.back().get(), boost::bind(&CountDownLatch::CountDown, boost::ref(latch))); > Yes you're right. I can't think of a sure shot way to make sure that we don I still don't think that helps, because the RPCs will still end up read off the wire by the reactor thread and thus won't be pending in the client outbound queue, even if the server is blocked _processing them_. You'd have to somehow block the server's reactor thread so that it isn't reading off the pipe, and then you'd have to fill up the socket's send-buffer so that the client can't make progress sending the transfers to the pipe -- To view, visit http://gerrit.cloudera.org:8080/9343 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iae1a5fe0066adf644a9cac41ad6696e1bbf00465 Gerrit-Change-Number: 9343 Gerrit-PatchSet: 6 Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Tue, 20 Feb 2018 21:55:09 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2301: (Part-1) Add instrumentation on a per connection level
Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/9343 ) Change subject: KUDU-2301: (Part-1) Add instrumentation on a per connection level .. Patch Set 6: > Build Failed > > http://jenkins.kudu.apache.org/job/kudu-gerrit/12071/ : FAILURE Failure is in an unrelated flaky test. -- To view, visit http://gerrit.cloudera.org:8080/9343 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iae1a5fe0066adf644a9cac41ad6696e1bbf00465 Gerrit-Change-Number: 9343 Gerrit-PatchSet: 6 Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Tue, 20 Feb 2018 21:45:42 + Gerrit-HasComments: No
[kudu-CR] KUDU-2301: (Part-1) Add instrumentation on a per connection level
Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/9343 ) Change subject: KUDU-2301: (Part-1) Add instrumentation on a per connection level .. Patch Set 6: (1 comment) http://gerrit.cloudera.org:8080/#/c/9343/5/src/kudu/rpc/rpc-test.cc File src/kudu/rpc/rpc-test.cc: http://gerrit.cloudera.org:8080/#/c/9343/5/src/kudu/rpc/rpc-test.cc@489 PS5, Line 489: controllers.back().get(), boost::bind(&CountDownLatch::CountDown, boost::ref(latch))); > Maybe I'm missing something here. If the main thread runs a bit slow before Yes you're right. I can't think of a sure shot way to make sure that we don't have this race. However, I've changed the RPC method to GenericCalculatorService::kSleepMethodName, with a 1 second sleep and 10 total RPCs. With 3 service threads (the default), that should give the main thread a little more than 3 seconds to dump the running RPCs information, which makes the flakiness very unlikely. I'm open to suggestions if this doesn't suffice. -- To view, visit http://gerrit.cloudera.org:8080/9343 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iae1a5fe0066adf644a9cac41ad6696e1bbf00465 Gerrit-Change-Number: 9343 Gerrit-PatchSet: 6 Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Tue, 20 Feb 2018 21:21:46 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2301: (Part-1) Add instrumentation on a per connection level
Hello Michael Ho, Lars Volker, Alexey Serbin, Dan Burkert, Kudu Jenkins, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/9343 to look at the new patch set (#6). Change subject: KUDU-2301: (Part-1) Add instrumentation on a per connection level .. KUDU-2301: (Part-1) Add instrumentation on a per connection level This patch returns the OutboundTransfer queue size on a per connection level and makes them accessible via the DumpRunningRpcs() call. A test is added in rpc-test to ensure that this metric works as expected. A future patch will add more metrics. Change-Id: Iae1a5fe0066adf644a9cac41ad6696e1bbf00465 --- M src/kudu/rpc/connection.cc M src/kudu/rpc/connection.h M src/kudu/rpc/messenger.h M src/kudu/rpc/rpc-test.cc M src/kudu/rpc/rpc_introspection.proto 5 files changed, 66 insertions(+), 3 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/43/9343/6 -- To view, visit http://gerrit.cloudera.org:8080/9343 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Iae1a5fe0066adf644a9cac41ad6696e1bbf00465 Gerrit-Change-Number: 9343 Gerrit-PatchSet: 6 Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-2301: (Part-1) Add instrumentation on a per connection level
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/9343 ) Change subject: KUDU-2301: (Part-1) Add instrumentation on a per connection level .. Patch Set 5: (1 comment) http://gerrit.cloudera.org:8080/#/c/9343/5/src/kudu/rpc/rpc-test.cc File src/kudu/rpc/rpc-test.cc: http://gerrit.cloudera.org:8080/#/c/9343/5/src/kudu/rpc/rpc-test.cc@489 PS5, Line 489: ASSERT_GT(dump_resp.outbound_connections(0).outbound_queue_size(), 0); Maybe I'm missing something here. If the main thread runs a bit slow before DumpRunningRpcs calls, what is preventing the queue from immediately emptying before this assertion? eg if you added a sleep on line 486 wouldn't this test now fail? Even with 1000 RPCs it seems like they could very easily all get sent in a matter of a couple milliseconds -- To view, visit http://gerrit.cloudera.org:8080/9343 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iae1a5fe0066adf644a9cac41ad6696e1bbf00465 Gerrit-Change-Number: 9343 Gerrit-PatchSet: 5 Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Tue, 20 Feb 2018 20:29:12 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2301: (Part-1) Add instrumentation on a per connection level
Hello Michael Ho, Lars Volker, Alexey Serbin, Dan Burkert, Kudu Jenkins, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/9343 to look at the new patch set (#5). Change subject: KUDU-2301: (Part-1) Add instrumentation on a per connection level .. KUDU-2301: (Part-1) Add instrumentation on a per connection level This patch returns the OutboundTransfer queue size on a per connection level and makes them accessible via the DumpRunningRpcs() call. A test is added in rpc-test to ensure that this metric works as expected. A future patch will add more metrics. Change-Id: Iae1a5fe0066adf644a9cac41ad6696e1bbf00465 --- M src/kudu/rpc/connection.cc M src/kudu/rpc/connection.h M src/kudu/rpc/messenger.h M src/kudu/rpc/rpc-test.cc M src/kudu/rpc/rpc_introspection.proto 5 files changed, 61 insertions(+), 3 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/43/9343/5 -- To view, visit http://gerrit.cloudera.org:8080/9343 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Iae1a5fe0066adf644a9cac41ad6696e1bbf00465 Gerrit-Change-Number: 9343 Gerrit-PatchSet: 5 Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-2301: (Part-1) Add instrumentation on a per connection level
Hello Michael Ho, Lars Volker, Alexey Serbin, Dan Burkert, Kudu Jenkins, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/9343 to look at the new patch set (#4). Change subject: KUDU-2301: (Part-1) Add instrumentation on a per connection level .. KUDU-2301: (Part-1) Add instrumentation on a per connection level This patch returns the OutboundTransfer queue size on a per connection level and makes them accessible via the DumpRunningRpcs() call. A test is added in rpc-test to ensure that this metric works as expected. A future patch will add more metrics. Change-Id: Iae1a5fe0066adf644a9cac41ad6696e1bbf00465 --- M src/kudu/rpc/connection.cc M src/kudu/rpc/connection.h M src/kudu/rpc/messenger.h M src/kudu/rpc/rpc-test.cc M src/kudu/rpc/rpc_introspection.proto 5 files changed, 60 insertions(+), 3 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/43/9343/4 -- To view, visit http://gerrit.cloudera.org:8080/9343 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Iae1a5fe0066adf644a9cac41ad6696e1bbf00465 Gerrit-Change-Number: 9343 Gerrit-PatchSet: 4 Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-2301: (Part-1) Add instrumentation on a per connection level
Sailesh Mukil has posted comments on this change. (
http://gerrit.cloudera.org:8080/9343 )
Change subject: KUDU-2301: (Part-1) Add instrumentation on a per connection
level
..
Patch Set 3:
(17 comments)
Since there's a lot of comments regarding the KB/s calculation and the rolling
window, I've removed that from this patch and will address all those comments
in a Part-2 patch.
This patch now only contains the outbound queue size metric. This is so that we
can get this patch in quickly and have some discussion before finalizing on
Part-2.
http://gerrit.cloudera.org:8080/#/c/9343/3//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/9343/3//COMMIT_MSG@7
PS3, Line 7: metrics
> Maybe find a different word than "metrics" which already means a particular
Done
http://gerrit.cloudera.org:8080/#/c/9343/3//COMMIT_MSG@7
PS3, Line 7: KUDU-2031
> This one looks wrong.
Done
http://gerrit.cloudera.org:8080/#/c/9343/3//COMMIT_MSG@9
PS3, Line 9: rolling
: average of transfer speeds
> Can you point out where this would be helpful (and where it won't)? Naively
Will address in part-2
http://gerrit.cloudera.org:8080/#/c/9343/3/src/kudu/rpc/connection.h
File src/kudu/rpc/connection.h:
http://gerrit.cloudera.org:8080/#/c/9343/3/src/kudu/rpc/connection.h@241
PS3, Line 241: double rough_kbps() const {
> why not to move the implementation into the connection.cc file?
Will address in part-2
http://gerrit.cloudera.org:8080/#/c/9343/3/src/kudu/rpc/connection.h@245
PS3, Line 245: for (int i = 0; i < rolling_window_size; ++i) {
: rough_kbps += rolling_kbps_[i];
: }
> syntactic sugar nit: I think it might be just
Will address in part-2
http://gerrit.cloudera.org:8080/#/c/9343/3/src/kudu/rpc/connection.h@398
PS3, Line 398: kbps
> Is this bit per second or byte per second? Please don't use abbreviations
Will address in part-2
http://gerrit.cloudera.org:8080/#/c/9343/3/src/kudu/rpc/connection.h@398
PS3, Line 398: kbps
> Can you talk a bit more about the use case for this? Do you think something
Will address in part-2
http://gerrit.cloudera.org:8080/#/c/9343/3/src/kudu/rpc/connection.cc
File src/kudu/rpc/connection.cc:
http://gerrit.cloudera.org:8080/#/c/9343/3/src/kudu/rpc/connection.cc@82
PS3, Line 82: rolling_kbps_.set_capacity(1000);
> move this to initialization list rolling_kbps_(1000) ?
Will address in part-2
http://gerrit.cloudera.org:8080/#/c/9343/3/src/kudu/rpc/connection.cc@82
PS3, Line 82: 1000
> Does it make sense to control the size of the rolling window by a gflag? T
Will address in part-2
http://gerrit.cloudera.org:8080/#/c/9343/3/src/kudu/rpc/connection.cc@662
PS3, Line 662: // Do the transfer and time it.
: Stopwatch send_timer;
: send_timer.start();
: Status status = transfer->SendBuffer(*socket_);
: send_timer.stop();
> agreed grabbing the time at the start of each transfer and then the time wh
Will address in part-2
http://gerrit.cloudera.org:8080/#/c/9343/3/src/kudu/rpc/connection.cc@662
PS3, Line 662: // Do the transfer and time it.
: Stopwatch send_timer;
: send_timer.start();
: Status status = transfer->SendBuffer(*socket_);
: send_timer.stop();
> Yeah I think there's a danger that this is just timing how long it takes to
Will address in part-2
http://gerrit.cloudera.org:8080/#/c/9343/3/src/kudu/rpc/connection.cc@662
PS3, Line 662: // Do the transfer and time it.
: Stopwatch send_timer;
: send_timer.start();
: Status status = transfer->SendBuffer(*socket_);
: send_timer.stop();
> Not sure how accurate this will be fore inferring throughput given this is
Will address in part-2
http://gerrit.cloudera.org:8080/#/c/9343/3/src/kudu/rpc/rpc-test.cc
File src/kudu/rpc/rpc-test.cc:
http://gerrit.cloudera.org:8080/#/c/9343/3/src/kudu/rpc/rpc-test.cc@471
PS3, Line 471: CHECK_OK
> why not just ASSERT_OK?
Will address in part-2
http://gerrit.cloudera.org:8080/#/c/9343/3/src/kudu/rpc/rpc-test.cc@472
PS3, Line 472: CHECK_OK
> ditto
Will address in part-2
http://gerrit.cloudera.org:8080/#/c/9343/3/src/kudu/rpc/rpc-test.cc@502
PS3, Line 502: p.AsyncRequest(GenericCalculatorService::kAddMethodName,
add_req, &add_resp,
: controllers.back().get(),
boost::bind(&CountDownLatch::CountDown, boost::ref(latch)));
> we tend to use std::bind instead of boost::bind in newer tests, if possible
I'd have to change the signature of ResponseCallback in that case. Is that
acceptable?
https://github.com/apache/kudu/blob/0ce5ba594412de4365625485ea7b3c1ee21bf28d/src/kudu/rpc/response_callback.h#L26
http://gerrit.cloudera.org:8080/#/c/9343/3/src/kudu/rpc/transfer.cc
File src/kudu/rpc/transfer.cc:
http://gerrit.cloudera.org:8080/#/c/9343
