[kudu-CR] KUDU-2031: Add metrics per connection to the reactor metrics

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9343 )

Change subject: KUDU-2031: Add metrics per connection to the reactor metrics
..


Patch Set 3:

(2 comments)

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@398
PS3, Line 398: kbps
> Is this bit per second or byte per second?  Please don't use abbreviations
Can you talk a bit more about the use case for this? Do you think something 
like exponential weighted moving average could get you something equally as 
useful with significantly less memory?


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@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
agreed grabbing the time at the start of each transfer and then the time when 
it finished (which may be a diferent iteration of the reactor loop) will more 
accurately capture this.



--
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 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Sat, 17 Feb 2018 02:22:45 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2031: Add metrics per connection to the reactor metrics

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9343 )

Change subject: KUDU-2031: Add metrics per connection to the reactor metrics
..


Patch Set 3:

(7 comments)

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?


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

  rough_kbps = std::accumulate(rolling_kbps_.begin(), rolling_kbps_.end(), 0.0);


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 in 
the comment to keep it clear.


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: 1000
Does it make sense to control the size of the rolling window by a gflag?  That 
way it would be possible to update it in run time using 
GenericServiceImpl::SetFlag(), at least, in Kudu.


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?


http://gerrit.cloudera.org:8080/#/c/9343/3/src/kudu/rpc/rpc-test.cc@472
PS3, Line 472: CHECK_OK
ditto


http://gerrit.cloudera.org:8080/#/c/9343/3/src/kudu/rpc/rpc-test.cc@502
PS3, Line 502: p.AsyncRequest(GenericCalculatorService::kAddMethodName, 
add_req, _resp,
 : controllers.back().get(), 
boost::bind(::CountDown, boost::ref(latch)));
we tend to use std::bind instead of boost::bind in newer tests, if possible



--
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 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Sat, 17 Feb 2018 01:04:36 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2031: Add metrics per connection to the reactor metrics

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9343 )

Change subject: KUDU-2031: Add metrics per connection to the reactor metrics
..


Patch Set 3:

(2 comments)

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@25
PS3, Line 25: #include 
prefer using 


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@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
Yeah I think there's a danger that this is just timing how long it takes to 
copy the bytes to the nic or tcp send buffer.  It may be more accurate to start 
the timer beginning of the transfer and stop it after it's completely sent, 
however that's still going to be inflated for small messages.

I looked a little bit into this, and it seems like we may be able to get the 
info we want from getsockopt/TCP_INFO, however you will need to keep this 
rolling-window setup since it's just a point in time snapshot.  
https://linuxgazette.net/136/pfeiffer.html has a good writeup.



--
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 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Fri, 16 Feb 2018 18:14:44 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2031: Add metrics per connection to the reactor metrics

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9343 )

Change subject: KUDU-2031: Add metrics per connection to the reactor metrics
..


Patch Set 3:

(1 comment)

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) ?



--
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 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Fri, 16 Feb 2018 01:44:40 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2031: Add metrics per connection to the reactor metrics

[Lars Volker (Code Review)]

Lars Volker has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9343 )

Change subject: KUDU-2031: Add metrics per connection to the reactor metrics
..


Patch Set 3:

(3 comments)

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 
class/thing?


http://gerrit.cloudera.org:8080/#/c/9343/3//COMMIT_MSG@7
PS3, Line 7: KUDU-2031
This one looks wrong.


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 
speaking, if a connection get's to send data very rarely, but then at high 
speeds, it will look like it's fast when it isn't. Should this actually 
consider the intervals between sends?



--
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 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Fri, 16 Feb 2018 01:40:57 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2031: Add metrics per connection to the reactor metrics

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9343 )

Change subject: KUDU-2031: Add metrics per connection to the reactor metrics
..


Patch Set 3:

(3 comments)

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@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 
non-blocking.

Also, will it be sufficient to approximate the Kbps based on the aggregate of 
all loop iterations instead of doing it per loop iteration ?


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.


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

int32_t total_remaining = payload_slices_[cur_slice_idx_].size() - 
cur_offset_in_slice_;



--
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 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Fri, 16 Feb 2018 01:32:27 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2031: Add metrics per connection to the reactor metrics

[Sailesh Mukil (Code Review)]

Hello Michael Ho, Lars Volker, 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 (#3).

Change subject: KUDU-2031: Add metrics per connection to the reactor metrics
..

KUDU-2031: Add metrics per connection to the reactor metrics

This patch returns the OutboundTransfer queue size and a rolling
average of transfer speeds in Kbps on a per connection level and
makes them accessible via the DumpRunningRpcs() call.

A test is added in rpc-test to ensure that these metrics work as
expected.

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
M src/kudu/rpc/transfer.cc
M src/kudu/rpc/transfer.h
7 files changed, 125 insertions(+), 3 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/43/9343/3
--
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: 3
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-2031: Add metrics per connection to the reactor metrics

[Sailesh Mukil (Code Review)]

Hello Michael Ho, Lars Volker, 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 (#2).

Change subject: KUDU-2031: Add metrics per connection to the reactor metrics
..

KUDU-2031: Add metrics per connection to the reactor metrics

This patch returns the OutboundTransfer queue size and a rolling
average of transfer speeds in Kbps on a per connection level and
makes them accessible via the DumpRunningRpcs() call.

A test is added in rpc-test to ensure that these metrics work as
expected.

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
M src/kudu/rpc/transfer.cc
M src/kudu/rpc/transfer.h
7 files changed, 125 insertions(+), 2 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/43/9343/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: newpatchset
Gerrit-Change-Id: Iae1a5fe0066adf644a9cac41ad6696e1bbf00465
Gerrit-Change-Number: 9343
Gerrit-PatchSet: 2
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-2031: Add metrics per connection to the reactor metrics

[Sailesh Mukil (Code Review)]

Sailesh Mukil has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/9343


Change subject: KUDU-2031: Add metrics per connection to the reactor metrics
..

KUDU-2031: Add metrics per connection to the reactor metrics

This patch returns the OutboundTransfer queue size and a rolling
average of transfer speeds in Kbps on a per connection level and
makes them accessible via the DumpRunningRpcs() call.

A test is added in rpc-test to ensure that these metrics work as
expected.

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
M src/kudu/rpc/transfer.cc
M src/kudu/rpc/transfer.h
7 files changed, 121 insertions(+), 1 deletion(-)



  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/43/9343/1
--
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: newchange
Gerrit-Change-Id: Iae1a5fe0066adf644a9cac41ad6696e1bbf00465
Gerrit-Change-Number: 9343
Gerrit-PatchSet: 1
Gerrit-Owner: Sailesh Mukil