Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/10540 )
Change subject: [tools] extra integration tests for the rebalancer ...................................................................... Patch Set 11: (4 comments) What did you learn after looping the new tests? http://gerrit.cloudera.org:8080/#/c/10540/10/src/kudu/tools/kudu-admin-test.cc File src/kudu/tools/kudu-admin-test.cc: http://gerrit.cloudera.org:8080/#/c/10540/10/src/kudu/tools/kudu-admin-test.cc@1711 PS10, Line 1711: } : } > The main purpose of this is to allow the rebalancer to run concurrently wit I agree that running the rebalancer in the main thread is nicer, but how is a SleepFor() between the two rebalancer runs any different than timer.join()? In both cases the main thread is blocked waiting for time to elapse, except the former is more direct than the latter. http://gerrit.cloudera.org:8080/#/c/10540/11/src/kudu/tools/kudu-admin-test.cc File src/kudu/tools/kudu-admin-test.cc: http://gerrit.cloudera.org:8080/#/c/10540/11/src/kudu/tools/kudu-admin-test.cc@1831 PS11, Line 1831: auto concurrent_runners_cleanup = MakeScopedCleanup([&]() { : start_synchronizer.CountDown(); : for (auto& runner : concurrent_runners) { : runner.join(); : } : }); : : // Run rebalancers concurrently. : start_synchronizer.CountDown(); : : // Wait for all rebalancers to complete. : for (auto& runner : concurrent_runners) { : runner.join(); : } : concurrent_runners_cleanup.cancel(); This is a strange pattern; it duplicates the cleanup code outright. Why is it necessary? I don't see anything that can fail between the creation of the scoped cleanup and the execution of the duplicated cleanup code, so what purpose does the scoped cleanup serve? Would it help if ScopedCleanup provided a way to execute the cleanup code now and prevent it from running again? http://gerrit.cloudera.org:8080/#/c/10540/11/src/kudu/tools/kudu-admin-test.cc@1930 PS11, Line 1930: RunTimeError Nit: RuntimeError http://gerrit.cloudera.org:8080/#/c/10540/11/src/kudu/tools/kudu-admin-test.cc@2029 PS11, Line 2029: RunTimeError Nit: RuntimeError. Also, can you decompose this into a helper and use it in the other test too? -- To view, visit http://gerrit.cloudera.org:8080/10540 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I78b3dcea71ed303f6ecd199604b2385796d05da8 Gerrit-Change-Number: 10540 Gerrit-PatchSet: 11 Gerrit-Owner: Alexey Serbin <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Will Berkeley <[email protected]> Gerrit-Comment-Date: Tue, 03 Jul 2018 21:48:57 +0000 Gerrit-HasComments: Yes
