You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Alexey Serbin (Code Review)" <ge...@cloudera.org> on 2020/06/11 04:20:40 UTC
[kudu-CR] [util] add BlockingQueueTest.MultiThreadPerf test scenario
Alexey Serbin has uploaded this change for review. ( http://gerrit.cloudera.org:8080/16064
Change subject: [util] add BlockingQueueTest.MultiThreadPerf test scenario
......................................................................
[util] add BlockingQueueTest.MultiThreadPerf test scenario
Added a new performance test scenario for BlockingQueue.
Change-Id: If52e4e15d3c72ad3479334f0f3fa8bb10b8c50c6
---
M src/kudu/util/blocking_queue-test.cc
1 file changed, 149 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/64/16064/1
--
To view, visit http://gerrit.cloudera.org:8080/16064
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: If52e4e15d3c72ad3479334f0f3fa8bb10b8c50c6
Gerrit-Change-Number: 16064
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
[kudu-CR] [util] add BlockingQueueTest.MultiThreadPerf test scenario
Posted by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org>.
Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/16064 )
Change subject: [util] add BlockingQueueTest.MultiThreadPerf test scenario
......................................................................
Patch Set 2: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/16064/1/src/kudu/util/blocking_queue-test.cc
File src/kudu/util/blocking_queue-test.cc:
http://gerrit.cloudera.org:8080/#/c/16064/1/src/kudu/util/blocking_queue-test.cc@45
PS1, Line 45: DEFINE_uint32(num_blocking_writers, 3,
: "number of threads calling BlockingQueue::BlockingPut()");
: DEFINE_uint32(num_non_blocking_writers, 2,
: "number of threads calling BlockingQueue::Put()");
: DEFINE_uint32(num_blocking_readers, 5,
: "number of threads calling BlockingQueue::BlockingGet()");
: DEFINE_uint32(runtime_sec, 5, "duration of the test (seconds)");
: DEFINE_uint32(queue_capacity, 64, "capacity of the queue (number of elements)");
> The reason I added these flags is to be able include this test into src/kud
Thanks for the explanation.
--
To view, visit http://gerrit.cloudera.org:8080/16064
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If52e4e15d3c72ad3479334f0f3fa8bb10b8c50c6
Gerrit-Change-Number: 16064
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 11 Jun 2020 20:11:37 +0000
Gerrit-HasComments: Yes
[kudu-CR] [util] add BlockingQueueTest.MultiThreadPerf test scenario
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/16064 )
Change subject: [util] add BlockingQueueTest.MultiThreadPerf test scenario
......................................................................
[util] add BlockingQueueTest.MultiThreadPerf test scenario
Added a new performance test scenario for BlockingQueue.
Change-Id: If52e4e15d3c72ad3479334f0f3fa8bb10b8c50c6
Reviewed-on: http://gerrit.cloudera.org:8080/16064
Reviewed-by: Bankim Bhavsar <ba...@cloudera.com>
Tested-by: Kudu Jenkins
Reviewed-by: Andrew Wong <aw...@cloudera.com>
---
M src/kudu/util/blocking_queue-test.cc
1 file changed, 155 insertions(+), 0 deletions(-)
Approvals:
Bankim Bhavsar: Looks good to me, approved
Kudu Jenkins: Verified
Andrew Wong: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/16064
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: If52e4e15d3c72ad3479334f0f3fa8bb10b8c50c6
Gerrit-Change-Number: 16064
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [util] add BlockingQueueTest.MultiThreadPerf test scenario
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Bankim Bhavsar, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16064
to look at the new patch set (#2).
Change subject: [util] add BlockingQueueTest.MultiThreadPerf test scenario
......................................................................
[util] add BlockingQueueTest.MultiThreadPerf test scenario
Added a new performance test scenario for BlockingQueue.
Change-Id: If52e4e15d3c72ad3479334f0f3fa8bb10b8c50c6
---
M src/kudu/util/blocking_queue-test.cc
1 file changed, 155 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/64/16064/2
--
To view, visit http://gerrit.cloudera.org:8080/16064
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If52e4e15d3c72ad3479334f0f3fa8bb10b8c50c6
Gerrit-Change-Number: 16064
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [util] add BlockingQueueTest.MultiThreadPerf test scenario
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16064 )
Change subject: [util] add BlockingQueueTest.MultiThreadPerf test scenario
......................................................................
Patch Set 1:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/16064/1/src/kudu/util/blocking_queue-test.cc
File src/kudu/util/blocking_queue-test.cc:
http://gerrit.cloudera.org:8080/#/c/16064/1/src/kudu/util/blocking_queue-test.cc@45
PS1, Line 45: DEFINE_uint32(num_blocking_writers, 3,
: "number of threads calling BlockingQueue::BlockingPut()");
: DEFINE_uint32(num_non_blocking_writers, 2,
: "number of threads calling BlockingQueue::Put()");
: DEFINE_uint32(num_blocking_readers, 5,
: "number of threads calling BlockingQueue::BlockingGet()");
: DEFINE_uint32(runtime_sec, 5, "duration of the test (seconds)");
: DEFINE_uint32(queue_capacity, 64, "capacity of the queue (number of elements)");
> Defining flags that will only be used in tests looks unnecessary.
The reason I added these flags is to be able include this test into src/kudu/scripts/benchmarks.sh to run with the desired settings without recompiling the binary. These flags are used to exercise various workload scenarios, and that's the common approach for micro-benchmark tests in Kudu so far. I used them to run various load scenarios, trying many of them.
http://gerrit.cloudera.org:8080/#/c/16064/1/src/kudu/util/blocking_queue-test.cc@463
PS1, Line 463: LOG(INFO) << Substitute(
> nit: any reason to not log about the non-blocking Put throughput too?
Done
http://gerrit.cloudera.org:8080/#/c/16064/1/src/kudu/util/blocking_queue-test.cc@463
PS1, Line 463: LOG(INFO) << Substitute(
> +1
The nature of the non-blocking put method makes it's the fuzziest candidate as for the meaningful call rate. But I added it as well, as requested. In addition, I added another fuzzy metric: the total rate of calls.
http://gerrit.cloudera.org:8080/#/c/16064/1/src/kudu/util/blocking_queue-test.cc@468
PS1, Line 468: const size_t num_blocking_writers_;
: const size_t num_non_blocking_writers_;
: const size_t num_blocking_readers_;
: const MonoDelta runtime_;
: BlockingQueue<uint64_t> queue_;
: vector<thread> threads_;
: CountDownLatch barrier_;
> These variables can be private.
Indeed. Per Andrew's comments, I'm getting rid of the Run() method and moving these fields/members under the 'protected' section.
http://gerrit.cloudera.org:8080/#/c/16064/1/src/kudu/util/blocking_queue-test.cc@481
PS1, Line 481: MultiThreadPerfTest().Run();
> nit: just curious, why bother encapsulating the run like this instead of wr
I was following the pattern established above in the BlockingQueueTest.TestMultipleThreads scenario :)
I agree, separating it into a dedicated method doesn't serve any functional purpose. I removed the method.
--
To view, visit http://gerrit.cloudera.org:8080/16064
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If52e4e15d3c72ad3479334f0f3fa8bb10b8c50c6
Gerrit-Change-Number: 16064
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 11 Jun 2020 19:51:07 +0000
Gerrit-HasComments: Yes
[kudu-CR] [util] add BlockingQueueTest.MultiThreadPerf test scenario
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16064 )
Change subject: [util] add BlockingQueueTest.MultiThreadPerf test scenario
......................................................................
Patch Set 2: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/16064
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If52e4e15d3c72ad3479334f0f3fa8bb10b8c50c6
Gerrit-Change-Number: 16064
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 11 Jun 2020 21:42:34 +0000
Gerrit-HasComments: No
[kudu-CR] [util] add BlockingQueueTest.MultiThreadPerf test scenario
Posted by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org>.
Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/16064 )
Change subject: [util] add BlockingQueueTest.MultiThreadPerf test scenario
......................................................................
Patch Set 1:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/16064/1/src/kudu/util/blocking_queue-test.cc
File src/kudu/util/blocking_queue-test.cc:
http://gerrit.cloudera.org:8080/#/c/16064/1/src/kudu/util/blocking_queue-test.cc@45
PS1, Line 45: DEFINE_uint32(num_blocking_writers, 3,
: "number of threads calling BlockingQueue::BlockingPut()");
: DEFINE_uint32(num_non_blocking_writers, 2,
: "number of threads calling BlockingQueue::Put()");
: DEFINE_uint32(num_blocking_readers, 5,
: "number of threads calling BlockingQueue::BlockingGet()");
: DEFINE_uint32(runtime_sec, 5, "duration of the test (seconds)");
: DEFINE_uint32(queue_capacity, 64, "capacity of the queue (number of elements)");
Defining flags that will only be used in tests looks unnecessary.
static constexpr/const variables in class will be better.
http://gerrit.cloudera.org:8080/#/c/16064/1/src/kudu/util/blocking_queue-test.cc@463
PS1, Line 463: LOG(INFO) << Substitute(
> nit: any reason to not log about the non-blocking Put throughput too?
+1
http://gerrit.cloudera.org:8080/#/c/16064/1/src/kudu/util/blocking_queue-test.cc@468
PS1, Line 468: const size_t num_blocking_writers_;
: const size_t num_non_blocking_writers_;
: const size_t num_blocking_readers_;
: const MonoDelta runtime_;
: BlockingQueue<uint64_t> queue_;
: vector<thread> threads_;
: CountDownLatch barrier_;
These variables can be private.
Similarly except ctor and Run(), other functions can be private as well.
--
To view, visit http://gerrit.cloudera.org:8080/16064
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If52e4e15d3c72ad3479334f0f3fa8bb10b8c50c6
Gerrit-Change-Number: 16064
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 11 Jun 2020 17:18:10 +0000
Gerrit-HasComments: Yes
[kudu-CR] [util] add BlockingQueueTest.MultiThreadPerf test scenario
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16064 )
Change subject: [util] add BlockingQueueTest.MultiThreadPerf test scenario
......................................................................
Patch Set 1: Code-Review+1
(2 comments)
http://gerrit.cloudera.org:8080/#/c/16064/1/src/kudu/util/blocking_queue-test.cc
File src/kudu/util/blocking_queue-test.cc:
http://gerrit.cloudera.org:8080/#/c/16064/1/src/kudu/util/blocking_queue-test.cc@463
PS1, Line 463: LOG(INFO) << Substitute(
nit: any reason to not log about the non-blocking Put throughput too?
http://gerrit.cloudera.org:8080/#/c/16064/1/src/kudu/util/blocking_queue-test.cc@481
PS1, Line 481: MultiThreadPerfTest().Run();
nit: just curious, why bother encapsulating the run like this instead of writing Run() into this test method directly?
--
To view, visit http://gerrit.cloudera.org:8080/16064
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If52e4e15d3c72ad3479334f0f3fa8bb10b8c50c6
Gerrit-Change-Number: 16064
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 11 Jun 2020 12:53:51 +0000
Gerrit-HasComments: Yes