Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15896 )
Change subject: [tests] added more tests for GetTableLocations() ...................................................................... Patch Set 1: (5 comments) http://gerrit.cloudera.org:8080/#/c/15896/1/src/kudu/integration-tests/table_locations-itest.cc File src/kudu/integration-tests/table_locations-itest.cc: http://gerrit.cloudera.org:8080/#/c/15896/1/src/kudu/integration-tests/table_locations-itest.cc@502 PS1, Line 502: while (!stop) { : GetTableLocationsRequestPB req; : GetTableLocationsResponsePB resp; : req.mutable_table()->set_table_name(table_name); : req.set_max_returned_locations(1000); : req.set_intern_ts_infos_in_response(true); : ++req_counters[idx]; : { : CatalogManager::ScopedLeaderSharedLock l(cm); : auto s = cm->GetTableLocations(&req, &resp, username); : if (!s.ok()) { : ++err_counters[idx]; : } : } nit: If we encapsulated this in a functor and reuse the rest of GetTableLocationsBenchmark? Do you foresee us benchmarking more catalog manager calls? http://gerrit.cloudera.org:8080/#/c/15896/1/src/kudu/integration-tests/table_locations-itest.cc@550 PS1, Line 550: lenght nit: length http://gerrit.cloudera.org:8080/#/c/15896/1/src/kudu/integration-tests/table_locations-itest.cc@554 PS1, Line 554: once fetched data. nit: "only after data is fetched"? http://gerrit.cloudera.org:8080/#/c/15896/1/src/kudu/integration-tests/table_locations-itest.cc@599 PS1, Line 599: // Multiple workloads vs single workload with multiple read threads: it's : // important to have multiple workloads to simulate multiple clients. A : // workload shares KuduClient between multiple read threads. Each client : // maintains its own table locations cache. nit: I found this a little confusing because the workloads below use a single read thread. Maybe just: "It's important to have multiple workloads to simulate multiple clients, each with its own table locations cache." http://gerrit.cloudera.org:8080/#/c/15896/1/src/kudu/integration-tests/table_locations-itest.cc@631 PS1, Line 631: hist->histogram()->DumpHumanReadable(&LOG(INFO)); nit: maybe also log what the histogram is showing, since the histogram itself doesn't have any descriptive info on what its contents represent. -- To view, visit http://gerrit.cloudera.org:8080/15896 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I61e413533bee2fa22f9e81531aadbea9e59ce6e2 Gerrit-Change-Number: 15896 Gerrit-PatchSet: 1 Gerrit-Owner: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Bankim Bhavsar <ban...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Mon, 11 May 2020 19:00:42 +0000 Gerrit-HasComments: Yes
