Abhishek Chennaka has posted comments on this change. ( http://gerrit.cloudera.org:8080/17989 )
Change subject: KUDU-1959 - Implement tests for the startup web page for Kudu Master ...................................................................... Patch Set 4: (10 comments) http://gerrit.cloudera.org:8080/#/c/17989/4/src/kudu/master/master-test.cc File src/kudu/master/master-test.cc: http://gerrit.cloudera.org:8080/#/c/17989/4/src/kudu/master/master-test.cc@381 PS4, Line 381: is_started > nit: taking into consideration what the thread is doing, maybe rename this Done http://gerrit.cloudera.org:8080/#/c/17989/4/src/kudu/master/master-test.cc@387 PS4, Line 387: 10 > nit: move these to a constant Done http://gerrit.cloudera.org:8080/#/c/17989/4/src/kudu/master/master-test.cc@387 PS4, Line 387: SleepFor(MonoDelta::FromMilliseconds(10)); > nit: given the actual start of the mini_master happens after this thread is Done http://gerrit.cloudera.org:8080/#/c/17989/4/src/kudu/master/master-test.cc@390 PS4, Line 390: 100 > Is it possible that this ill ever be 0? Or does it get set strictly before Yes, it is possible. Great catch there. Must be an error on my side. Fixed it. http://gerrit.cloudera.org:8080/#/c/17989/4/src/kudu/master/master-test.cc@390 PS4, Line 390: 100 > As Andrew pointed, it would be great to clarify on the init_status (maybe, Done http://gerrit.cloudera.org:8080/#/c/17989/4/src/kudu/master/master-test.cc@390 PS4, Line 390: 100 > Assuming the order doesn't matter, you could also move these all to separat I felt calling the function once with a longer string to match would be cheaper than calling the function multiple times with a shorter string? Maybe similar to the usage in src/kudu/tools/kudu-txn-cli-test.cc http://gerrit.cloudera.org:8080/#/c/17989/4/src/kudu/master/master-test.cc@394 PS4, Line 394: [0-9]|[1-9][0-9]|100 > Any particular reason why not simply [0-9]{1,3} for all of these? Yes, I thought about that approach too but we would want it to be strictly between [0,100] and not more than 100, hence the above approach. http://gerrit.cloudera.org:8080/#/c/17989/4/src/kudu/master/master-test.cc@405 PS4, Line 405: SCOPED_CLEANUP({ : is_started = true; : read_startup_page.join(); : }); > What happens if one of the two ASSERT_OK() above triggers? In other words, Done http://gerrit.cloudera.org:8080/#/c/17989/4/src/kudu/master/master-test.cc@405 PS4, Line 405: SCOPED_CLEANUP({ : is_started = true; : read_startup_page.join(); : }); > What does 'aborts' mean here? Doesn't the test framework continues running Guess Alexey meant to use something like: SCOPED_CLEANUP({ read_startup_page.join(); }); ASSERT_OK(mini_master_->Restart()); ASSERT_OK(mini_master_->WaitForCatalogManagerInit()); run_status_reader = true; http://gerrit.cloudera.org:8080/#/c/17989/4/src/kudu/master/master.cc File src/kudu/master/master.cc: http://gerrit.cloudera.org:8080/#/c/17989/4/src/kudu/master/master.cc@265 PS4, Line 265: startup_path_handler_->initialize_master_catalog_progress()->Start(); > Is it possible to move both Start() and Stop() for this timer into Master:: Makes sense. Done. -- To view, visit http://gerrit.cloudera.org:8080/17989 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I823a59df215cfeb579f1593a63ef6133de37271c Gerrit-Change-Number: 17989 Gerrit-PatchSet: 4 Gerrit-Owner: Abhishek Chennaka <[email protected]> Gerrit-Reviewer: Abhishek Chennaka <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Mon, 08 Nov 2021 03:40:14 +0000 Gerrit-HasComments: Yes
