Attila Bukor 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: (3 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@390 PS4, Line 390: > 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? That makes sense, but I think readability and flexibility is more important than performance in test code, at least to a degree. It's possible, for example, that someone decides that these should be in alphabetical order, or wants to put add a new metric somewhere between these lines as their logical place. I'm not sure if a change like that should break this test. http://gerrit.cloudera.org:8080/#/c/17989/4/src/kudu/master/master-test.cc@394 PS4, Line 394: *\"read_data_directo > Yes, I thought about that approach too but we would want it to be strictly Right after these you have .*, so this would match 1000 as well, so I still don't really see the upside of having the more complicated regex. 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@407 PS5, Line 407: run_status_reader = true; If the above two assertions fail, this line will be never reached and the thread read_startup_page thread will loop infinitely. I think it's best to copy this statement to scoped_cleanup as well before the join. -- 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 16:01:29 +0000 Gerrit-HasComments: Yes
