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 <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Bankim Bhavsar <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Comment-Date: Thu, 11 Jun 2020 19:51:07 +0000 Gerrit-HasComments: Yes
