You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Todd Lipcon (Code Review)" <ge...@cloudera.org> on 2019/02/27 00:15:47 UTC
[kudu-CR] KUDU-2711 (part 1): add a benchmark for GetTableLocations
Hello Will Berkeley,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/12613
to review the following change.
Change subject: KUDU-2711 (part 1): add a benchmark for GetTableLocations
......................................................................
KUDU-2711 (part 1): add a benchmark for GetTableLocations
This adds a simple benchmark which spawns a bunch of threads which
hammer GetTableLocations in a loop. Upon completion, it dumps out
a histogram.
Change-Id: Ic1643638d82b7697d20ea960592c8b70c30ac7bd
---
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/linked_list-test-util.h
M src/kudu/integration-tests/table_locations-itest.cc
M src/kudu/util/hdr_histogram.cc
M src/kudu/util/hdr_histogram.h
5 files changed, 101 insertions(+), 17 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/13/12613/1
--
To view, visit http://gerrit.cloudera.org:8080/12613
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ic1643638d82b7697d20ea960592c8b70c30ac7bd
Gerrit-Change-Number: 12613
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] KUDU-2711 (part 1): add a benchmark for GetTableLocations
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/12613 )
Change subject: KUDU-2711 (part 1): add a benchmark for GetTableLocations
......................................................................
Patch Set 3: Verified+1
Client connection metrics flake.
--
To view, visit http://gerrit.cloudera.org:8080/12613
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic1643638d82b7697d20ea960592c8b70c30ac7bd
Gerrit-Change-Number: 12613
Gerrit-PatchSet: 3
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Thu, 07 Mar 2019 21:29:00 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-2711 (part 1): add a benchmark for GetTableLocations
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12613 )
Change subject: KUDU-2711 (part 1): add a benchmark for GetTableLocations
......................................................................
Patch Set 1:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/12613/1/src/kudu/integration-tests/table_locations-itest.cc
File src/kudu/integration-tests/table_locations-itest.cc:
http://gerrit.cloudera.org:8080/#/c/12613/1/src/kudu/integration-tests/table_locations-itest.cc@326
PS1, Line 326: const int kNumReactors = 8;
Curious why not have kNumThreads == kNumReactors?
http://gerrit.cloudera.org:8080/#/c/12613/1/src/kudu/integration-tests/table_locations-itest.cc@383
PS1, Line 383: &LOG(INFO)
Neat, didn't know you could get an ostream that way.
http://gerrit.cloudera.org:8080/#/c/12613/1/src/kudu/util/hdr_histogram.h
File src/kudu/util/hdr_histogram.h:
http://gerrit.cloudera.org:8080/#/c/12613/1/src/kudu/util/hdr_histogram.h@171
PS1, Line 171: void DumpHumanReadable(std::ostream* out) const;
Maybe some light documentation, since the rest of the functions are fairly well documented?
--
To view, visit http://gerrit.cloudera.org:8080/12613
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic1643638d82b7697d20ea960592c8b70c30ac7bd
Gerrit-Change-Number: 12613
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Wed, 27 Feb 2019 00:31:58 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2711 (part 1): add a benchmark for GetTableLocations
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12613 )
Change subject: KUDU-2711 (part 1): add a benchmark for GetTableLocations
......................................................................
Patch Set 2: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/12613
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic1643638d82b7697d20ea960592c8b70c30ac7bd
Gerrit-Change-Number: 12613
Gerrit-PatchSet: 2
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Wed, 06 Mar 2019 05:22:15 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-2711 (part 1): add a benchmark for GetTableLocations
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/12613 )
Change subject: KUDU-2711 (part 1): add a benchmark for GetTableLocations
......................................................................
Patch Set 1:
(1 comment)
I wrote an eerily similar patch yesterday (at least, the body of the test was very similar).
http://gerrit.cloudera.org:8080/#/c/12613/1/src/kudu/util/hdr_histogram.cc
File src/kudu/util/hdr_histogram.cc:
http://gerrit.cloudera.org:8080/#/c/12613/1/src/kudu/util/hdr_histogram.cc@328
PS1, Line 328: HdrHistogram::DumpHumanReadable
Consider using this to dump the histograms in rpc-bench.cc.
--
To view, visit http://gerrit.cloudera.org:8080/12613
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic1643638d82b7697d20ea960592c8b70c30ac7bd
Gerrit-Change-Number: 12613
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Wed, 27 Feb 2019 01:02:45 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2711 (part 1): add a benchmark for GetTableLocations
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has uploaded a new patch set (#3) to the change originally created by Todd Lipcon. ( http://gerrit.cloudera.org:8080/12613 )
Change subject: KUDU-2711 (part 1): add a benchmark for GetTableLocations
......................................................................
KUDU-2711 (part 1): add a benchmark for GetTableLocations
This adds a simple benchmark which spawns a bunch of threads which
hammer GetTableLocations in a loop. Upon completion, it dumps out
a histogram.
Change-Id: Ic1643638d82b7697d20ea960592c8b70c30ac7bd
---
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/linked_list-test-util.h
M src/kudu/integration-tests/table_locations-itest.cc
M src/kudu/rpc/rpc-bench.cc
M src/kudu/util/hdr_histogram.cc
M src/kudu/util/hdr_histogram.h
6 files changed, 113 insertions(+), 28 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/13/12613/3
--
To view, visit http://gerrit.cloudera.org:8080/12613
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic1643638d82b7697d20ea960592c8b70c30ac7bd
Gerrit-Change-Number: 12613
Gerrit-PatchSet: 3
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] KUDU-2711 (part 1): add a benchmark for GetTableLocations
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/12613 )
Change subject: KUDU-2711 (part 1): add a benchmark for GetTableLocations
......................................................................
KUDU-2711 (part 1): add a benchmark for GetTableLocations
This adds a simple benchmark which spawns a bunch of threads which
hammer GetTableLocations in a loop. Upon completion, it dumps out
a histogram.
Change-Id: Ic1643638d82b7697d20ea960592c8b70c30ac7bd
Reviewed-on: http://gerrit.cloudera.org:8080/12613
Reviewed-by: Adar Dembo <ad...@cloudera.com>
Tested-by: Will Berkeley <wd...@gmail.com>
---
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/linked_list-test-util.h
M src/kudu/integration-tests/table_locations-itest.cc
M src/kudu/rpc/rpc-bench.cc
M src/kudu/util/hdr_histogram.cc
M src/kudu/util/hdr_histogram.h
6 files changed, 113 insertions(+), 28 deletions(-)
Approvals:
Adar Dembo: Looks good to me, approved
Will Berkeley: Verified
--
To view, visit http://gerrit.cloudera.org:8080/12613
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ic1643638d82b7697d20ea960592c8b70c30ac7bd
Gerrit-Change-Number: 12613
Gerrit-PatchSet: 4
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] KUDU-2711 (part 1): add a benchmark for GetTableLocations
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/12613 )
Change subject: KUDU-2711 (part 1): add a benchmark for GetTableLocations
......................................................................
Patch Set 2:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/12613/1/src/kudu/integration-tests/table_locations-itest.cc
File src/kudu/integration-tests/table_locations-itest.cc:
http://gerrit.cloudera.org:8080/#/c/12613/1/src/kudu/integration-tests/table_locations-itest.cc@326
PS1, Line 326: const int kNumSplits = FLAGS_benchmark_num_tablets - 1;
> Curious why not have kNumThreads == kNumReactors?
I'm not sure what Todd's reason was, but I tested using a single messenger and #reactors = #threads and it seemed to perform roughly the same after some light testing.
http://gerrit.cloudera.org:8080/#/c/12613/1/src/kudu/integration-tests/table_locations-itest.cc@358
PS1, Line 358: for (int i = 0; i < kNumThreads; i++) {
> warning: 'emplace_back' is called inside a loop; consider pre-allocating th
Done
http://gerrit.cloudera.org:8080/#/c/12613/1/src/kudu/integration-tests/table_locations-itest.cc@383
PS1, Line 383:
> Neat, didn't know you could get an ostream that way.
:themoreyouknow:
http://gerrit.cloudera.org:8080/#/c/12613/1/src/kudu/util/hdr_histogram.h
File src/kudu/util/hdr_histogram.h:
http://gerrit.cloudera.org:8080/#/c/12613/1/src/kudu/util/hdr_histogram.h@168
PS1, Line 168: // TODO(mpercy): implement
> warning: missing username/bug in TODO [google-readability-todo]
Done
http://gerrit.cloudera.org:8080/#/c/12613/1/src/kudu/util/hdr_histogram.h@171
PS1, Line 171: // Dump a formatted, multiline string describing this histogram to 'out'.
> Maybe some light documentation, since the rest of the functions are fairly
Done
http://gerrit.cloudera.org:8080/#/c/12613/1/src/kudu/util/hdr_histogram.cc
File src/kudu/util/hdr_histogram.cc:
http://gerrit.cloudera.org:8080/#/c/12613/1/src/kudu/util/hdr_histogram.cc@328
PS1, Line 328: HdrHistogram::DumpHumanReadable
> Consider using this to dump the histograms in rpc-bench.cc.
Done
--
To view, visit http://gerrit.cloudera.org:8080/12613
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic1643638d82b7697d20ea960592c8b70c30ac7bd
Gerrit-Change-Number: 12613
Gerrit-PatchSet: 2
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Tue, 05 Mar 2019 20:12:10 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2711 (part 1): add a benchmark for GetTableLocations
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12613 )
Change subject: KUDU-2711 (part 1): add a benchmark for GetTableLocations
......................................................................
Patch Set 3: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/12613
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic1643638d82b7697d20ea960592c8b70c30ac7bd
Gerrit-Change-Number: 12613
Gerrit-PatchSet: 3
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Thu, 07 Mar 2019 19:01:31 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-2711 (part 1): add a benchmark for GetTableLocations
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has removed a vote on this change.
Change subject: KUDU-2711 (part 1): add a benchmark for GetTableLocations
......................................................................
Removed Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/12613
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: Ic1643638d82b7697d20ea960592c8b70c30ac7bd
Gerrit-Change-Number: 12613
Gerrit-PatchSet: 3
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] KUDU-2711 (part 1): add a benchmark for GetTableLocations
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/12613 )
Change subject: KUDU-2711 (part 1): add a benchmark for GetTableLocations
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12613/1/src/kudu/integration-tests/table_locations-itest.cc
File src/kudu/integration-tests/table_locations-itest.cc:
http://gerrit.cloudera.org:8080/#/c/12613/1/src/kudu/integration-tests/table_locations-itest.cc@326
PS1, Line 326: const int kNumSplits = FLAGS_benchmark_num_tablets - 1;
> I'm not sure what Todd's reason was, but I tested using a single messenger
I think I went with multiple whole messengers so that we end up with multiple TCP connections. A single TCP connection ends up on a single reactor regardless of the number of reactors available. Watching 'top -H' while the benchmark was running I saw that one of the reactors was at 100% CPU, meaning we were benchmarking the serialization/network code and not realistic to a normal workload with many inbound connections
--
To view, visit http://gerrit.cloudera.org:8080/12613
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic1643638d82b7697d20ea960592c8b70c30ac7bd
Gerrit-Change-Number: 12613
Gerrit-PatchSet: 2
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Wed, 06 Mar 2019 17:50:51 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2711 (part 1): add a benchmark for GetTableLocations
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/12613 )
Change subject: KUDU-2711 (part 1): add a benchmark for GetTableLocations
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12613/1/src/kudu/integration-tests/table_locations-itest.cc
File src/kudu/integration-tests/table_locations-itest.cc:
http://gerrit.cloudera.org:8080/#/c/12613/1/src/kudu/integration-tests/table_locations-itest.cc@326
PS1, Line 326: const int kNumReactors = 8;
> I think I went with multiple whole messengers so that we end up with multip
Gotcha, we should probably keep it straightforward and match the number of messengers to the number of threads, then.
--
To view, visit http://gerrit.cloudera.org:8080/12613
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic1643638d82b7697d20ea960592c8b70c30ac7bd
Gerrit-Change-Number: 12613
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Thu, 07 Mar 2019 18:14:14 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2711 (part 1): add a benchmark for GetTableLocations
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has uploaded a new patch set (#2) to the change originally created by Todd Lipcon. ( http://gerrit.cloudera.org:8080/12613 )
Change subject: KUDU-2711 (part 1): add a benchmark for GetTableLocations
......................................................................
KUDU-2711 (part 1): add a benchmark for GetTableLocations
This adds a simple benchmark which spawns a bunch of threads which
hammer GetTableLocations in a loop. Upon completion, it dumps out
a histogram.
Change-Id: Ic1643638d82b7697d20ea960592c8b70c30ac7bd
---
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/linked_list-test-util.h
M src/kudu/integration-tests/table_locations-itest.cc
M src/kudu/rpc/rpc-bench.cc
M src/kudu/util/hdr_histogram.cc
M src/kudu/util/hdr_histogram.h
6 files changed, 109 insertions(+), 28 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/13/12613/2
--
To view, visit http://gerrit.cloudera.org:8080/12613
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic1643638d82b7697d20ea960592c8b70c30ac7bd
Gerrit-Change-Number: 12613
Gerrit-PatchSet: 2
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>