Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/17990 )
Change subject: KUDU-1959 - Add tests for /startup page for tservers ...................................................................... Patch Set 6: (3 comments) 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: FLAGS_block_manager = GetParam(); IMO it doesn't seem worth parameterizing this test. We already get limited mileage because the FBM isn't used very much, but also, the differences in code paths are mostly unrelated to this parameterization in the first place -- only the LBM counts really change, and the change is the skipping of an entire test; the rest of the metric codepaths are basically the same across parameterizations. Instead, how about we just run all of these tests with the default block manager, and in TestLogBlockContainerMetrics, if the default block manager is "file" e.g. in MacOS, we change the things that we check for -- instead of checking for the presence of LBM metrics, we check for the _absence_ of the LBM metrics. http://gerrit.cloudera.org:8080/#/c/17990/6/src/kudu/tserver/tablet_server-test.cc@795 PS6, Line 795: current_status nit: maybe call this "metrics_page" or something, so it's a bit more obvious what it is expected to be, without having to read the rest of this file Same below http://gerrit.cloudera.org:8080/#/c/17990/6/src/kudu/tserver/tablet_server-test.cc@921 PS6, Line 921: CheckNonZeroMetricIsStable nit: for void functions that may raise assertions, surround them in NO_FATALS(), so the error message will point to this line, rather than to a line in CheckNonZeroMetricIsStable's defn Same elsewhere -- 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: 6 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, 08 Nov 2021 23:26:52 +0000 Gerrit-HasComments: Yes
