Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/15896 )
Change subject: [tests] added more tests for GetTableLocations() ...................................................................... Patch Set 1: (6 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 GetTableLoc Yup, it might be the case. As of now, the code is a bit different for each scenario. I'd start thinking about re-factoring this out into a function/functor once we have more cases like this. Meanwhile, I separated the common code into new method named CreateTables(). http://gerrit.cloudera.org:8080/#/c/15896/1/src/kudu/integration-tests/table_locations-itest.cc@550 PS1, Line 550: lenght > nit: length Done 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"? I updated the whole sentence: Make table location information expire faster so the clients would try to refresh the information during the scenario. http://gerrit.cloudera.org:8080/#/c/15896/1/src/kudu/integration-tests/table_locations-itest.cc@580 PS1, Line 580: for (auto idx = 0; idx < kNumThreads; ++idx) { : TestWorkload w(cluster_.get()); : w.set_num_replicas(3); : w.set_num_tablets(10); : w.set_table_name(Substitute("test_table_$0", idx)); : w.set_num_write_threads(1); : w.set_num_read_threads(0); : w.Setup(); : w.Start(); : while (w.rows_inserted() < 100) { : SleepFor(MonoDelta::FromMilliseconds(10)); : } : w.StopAndJoin(); : } > Could you add a comment what's going on here because I don't understand it. The idea was to create multiple test tables and populate them with some data. Done 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 sing Done 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 itse Done -- 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: 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 22:42:37 +0000 Gerrit-HasComments: Yes
