Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/8420 )
Change subject: Add a benchmark test for tablet deletion ...................................................................... Patch Set 2: (10 comments) http://gerrit.cloudera.org:8080/#/c/8420/1/src/kudu/tserver/tablet_server-test.cc File src/kudu/tserver/tablet_server-test.cc: http://gerrit.cloudera.org:8080/#/c/8420/1/src/kudu/tserver/tablet_server-test.cc@131 PS1, Line 131: DEFINE_int32(delete_tablet_bench_num_flushes, 200, : "Number of disk row sets to flush in the delete tablet benchmark"); : > Nit: the name and description of this new flag suggests that it's part of t Done http://gerrit.cloudera.org:8080/#/c/8420/1/src/kudu/tserver/tablet_server-test.cc@2293 PS1, Line 2293: uint64_t rows = FLAGS_delete_tablet_bench_num_flushes; > I don't think this is necessary; the other benchmark in this test isn't con Done http://gerrit.cloudera.org:8080/#/c/8420/1/src/kudu/tserver/tablet_server-test.cc@2295 PS1, Line 2295: // Collect some related metrics. : scoped_refptr<AtomicGauge<uint64_t>> block_count = > How long does this take to run in slow mode? If it's less than 5s, I don't With 'delete_tablet_bench_num_flushes' default to 200, the test ran for 3s. So I will remove it. http://gerrit.cloudera.org:8080/#/c/8420/1/src/kudu/tserver/tablet_server-test.cc@2299 PS1, Line 2299: scoped_refptr<AtomicGauge<uint64_t>> container = > Nit: terminate with a period. Done http://gerrit.cloudera.org:8080/#/c/8420/1/src/kudu/tserver/tablet_server-test.cc@2300 PS1, Line 2300: METRIC_log_block_manager_containers.Instantiate( > Isn't this guaranteed to be true by the test harness itself? Perhaps it can Done http://gerrit.cloudera.org:8080/#/c/8420/1/src/kudu/tserver/tablet_server-test.cc@2303 PS1, Line 2303: METRIC_log_block_manager_holes_punched.Instantiate( : mini_server_->server()->metric_entity()); : : // Put some data in the tablet. We insert rows and flush immediately to : // ensure that there is enough blocks on disk to run the benchmark. : for (int i = 0; i < rows; i++) { : NO_FATALS(InsertTestRowsRemote(i, 1)); : ASSERT_OK(tablet_replica_->tablet()->Flush()); : } > I presume this is okay to execute even when block_manager=file? Right, it is safe to do so. http://gerrit.cloudera.org:8080/#/c/8420/1/src/kudu/tserver/tablet_server-test.cc@2314 PS1, Line 2314: ablet from within > Nit: replace with "run the" Done http://gerrit.cloudera.org:8080/#/c/8420/1/src/kudu/tserver/tablet_server-test.cc@2316 PS1, Line 2316: // by the test code. > Nit: new code should use the shorter NO_FATALS() macro. Done http://gerrit.cloudera.org:8080/#/c/8420/1/src/kudu/tserver/tablet_server-test.cc@2335 PS1, Line 2335: // Log the related metrics. > No latency is being measured here. Done http://gerrit.cloudera.org:8080/#/c/8420/1/src/kudu/tserver/tablet_server-test.cc@2344 PS1, Line 2344: // Verify that the tablet exists > You can log this regardless of block manager, no? Right, will remove the condition. -- To view, visit http://gerrit.cloudera.org:8080/8420 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I6054b55f81e5a6e6febcbb5781ad8d776c49a5cf Gerrit-Change-Number: 8420 Gerrit-PatchSet: 2 Gerrit-Owner: Hao Hao <hao....@cloudera.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Hao Hao <hao....@cloudera.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Comment-Date: Tue, 31 Oct 2017 00:18:35 +0000 Gerrit-HasComments: Yes
