Alexey Serbin 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: (4 comments) A few more nits. 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 variable into 'run_status_reader' and negate current values? 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 created and started, you could move this SleepFor() to be the very first statement in the while() loop and remove the extra SleepFor() in the very end of the cycle. 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 As Andrew pointed, it would be great to clarify on the init_status (maybe, add a comment?). One more readability nit for the ASSERT_STR_MATCHES pattern: it would be great if the line breaks corresponded to the status/percentage pairs 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, consider moving this scoped cleanup right after instantiating 'read_startup_page' above and setting 'is_started' right after calling WaitForCatalogManagerInit() as well. -- 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: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 04 Nov 2021 02:25:08 +0000 Gerrit-HasComments: Yes
