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/3/src/kudu/rpc/transfer.cc@245 PS3, Line 245: if (TransferFinished()) return 0; > nit: Not sure if Kudu coding standard allows this kind of one liner. Will address in part-2 http://gerrit.cloudera.org:8080/#/c/9343/3/src/kudu/rpc/transfer.cc@246 PS3, Line 246: int32_t rem_in_cur_slice = payload_slices_[cur_slice_idx_].size() - cur_offset_in_slice_; : int32_t total_remaining = rem_in_cur_slice; > Why not just write Will address in part-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: 3 Gerrit-Owner: Sailesh Mukil <sail...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Dan Burkert <d...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Lars Volker <l...@cloudera.com> Gerrit-Reviewer: Michael Ho <k...@cloudera.com> Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com> Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-Comment-Date: Tue, 20 Feb 2018 01:39:56 +0000 Gerrit-HasComments: Yes
