Abhishek Chennaka has posted comments on this change. ( http://gerrit.cloudera.org:8080/17990 )
Change subject: KUDU-1959 - Add tests for /startup page and metrics for tservers ...................................................................... Patch Set 12: (10 comments) http://gerrit.cloudera.org:8080/#/c/17990/11//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/17990/11//COMMIT_MSG@14 PS11, Line 14: > nit: add a new line to separate the two lists + parenthesis should be moved Done http://gerrit.cloudera.org:8080/#/c/17990/6/src/kudu/tserver/tablet_server-test.cc File src/kudu/tserver/tablet_server-test.cc: http://gerrit.cloudera.org:8080/#/c/17990/6/src/kudu/tserver/tablet_server-test.cc@775 PS6, Line 775: NO_FATALS(StartTabletServer(kNumD > IMO it doesn't seem worth parameterizing this test. We already get limited Done http://gerrit.cloudera.org:8080/#/c/17990/6/src/kudu/tserver/tablet_server-test.cc@795 PS6, Line 795: e_status, "\"r > nit: maybe call this "metrics_page" or something, so it's a bit more obviou Done http://gerrit.cloudera.org:8080/#/c/17990/6/src/kudu/tserver/tablet_server-test.cc@921 PS6, Line 921: > nit: for void functions that may raise assertions, surround them in NO_FATA Done http://gerrit.cloudera.org:8080/#/c/17990/8/src/kudu/tserver/tablet_server-test.cc File src/kudu/tserver/tablet_server-test.cc: http://gerrit.cloudera.org:8080/#/c/17990/8/src/kudu/tserver/tablet_server-test.cc@793 PS8, Line 793: g& > nit: move the ampersand over, same below Done http://gerrit.cloudera.org:8080/#/c/17990/8/src/kudu/tserver/tablet_server-test.cc@798 PS8, Line 798: > nit: align spaces Done http://gerrit.cloudera.org:8080/#/c/17990/8/src/kudu/tserver/tablet_server-test.cc@829 PS8, Line 829: const string url = Substitute("http://$0/startup";, mini_s > nit: seems like this is only used in the context of the full URL. How about Done http://gerrit.cloudera.org:8080/#/c/17990/8/src/kudu/tserver/tablet_server-test.cc@833 PS8, Line 833: url, &b > nit: there is a using directive for this, so we can drop the "strings::" Done http://gerrit.cloudera.org:8080/#/c/17990/8/src/kudu/tserver/tablet_server-test.cc@852 PS8, Line 852: t > nit: can drop this outer parentheses Done http://gerrit.cloudera.org:8080/#/c/17990/8/src/kudu/tserver/tablet_server-test.cc@896 PS8, Line 896: // Validate populated metrics in case of zero tablets dur > nit: same here w.r.t the URL Done -- To view, visit http://gerrit.cloudera.org:8080/17990 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9f432b4eb813e51214b4d6b3c5b7b4c89426f47f Gerrit-Change-Number: 17990 Gerrit-PatchSet: 12 Gerrit-Owner: Abhishek Chennaka <[email protected]> Gerrit-Reviewer: Abhishek Chennaka <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Mon, 15 Nov 2021 21:49:00 +0000 Gerrit-HasComments: Yes
