Attila Bukor 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) 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@387 PS4, Line 387: 10 nit: move these to a constant 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, Assuming the order doesn't matter, you could also move these all to separate ASSERT_STR_MATCHES calls, one for each metric, similar to the ASSERT_STR_CONTAINS calls below. 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? 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, When an ASSERT_* fails, it aborts program execution. I wonder what happens if there are more asserts in the scoped cleanup in this case and if it's safe to use. If it isn't, you could split the OK and the failure case to two different flags like is_started and is_failed and in the failure case simply break out of the loop in the read_startup_page thread. -- 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: Attila Bukor <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Fri, 05 Nov 2021 09:18:56 +0000 Gerrit-HasComments: Yes
