Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/17989 )
Change subject: KUDU-1959 - Add tests for /startup page for the Master ...................................................................... Patch Set 5: (2 comments) http://gerrit.cloudera.org:8080/#/c/17989/5/src/kudu/master/master-test.cc File src/kudu/master/master-test.cc: http://gerrit.cloudera.org:8080/#/c/17989/5/src/kudu/master/master-test.cc@382 PS5, Line 382: const int wait_time = 10; nit: constant values are usually named per the GSG https://google.github.io/styleguide/cppguide.html#Constant_Names, i.e. kWaitTime That said, it does seems odd to have a separate constant for something used just once. In general we do prefer to not introduce "magic numbers" in tests, especially when used in several places. But in this case it's used just once, and a reader could infer its value is arbitrarily short. I.e. if the value had to be something more specific, a constant variable and/or a comment would be warranted. But to avoid a tight loop, IMO it's fairly self-explanatory. http://gerrit.cloudera.org:8080/#/c/17989/5/src/kudu/master/master-test.cc@407 PS5, Line 407: run_status_reader = true; > If the above two assertions fail, this line will be never reached and the t +1 -- 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: 5 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 23:04:34 +0000 Gerrit-HasComments: Yes
