Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/17990 )
Change subject: KUDU-1959 - Implement tests for the startup progress page for tablet servers ...................................................................... Patch Set 3: (8 comments) http://gerrit.cloudera.org:8080/#/c/17990/3/src/kudu/tserver/tablet_server-test.cc File src/kudu/tserver/tablet_server-test.cc: http://gerrit.cloudera.org:8080/#/c/17990/3/src/kudu/tserver/tablet_server-test.cc@31 PS3, Line 31: nit: remove the extra newline http://gerrit.cloudera.org:8080/#/c/17990/3/src/kudu/tserver/tablet_server-test.cc@46 PS3, Line 46: nit: remove the extra newlines http://gerrit.cloudera.org:8080/#/c/17990/3/src/kudu/tserver/tablet_server-test.cc@782 PS3, Line 782: FLAGS_tablet_bootstrap_inject_latency_ms I'm not sure there's much value in adding a test that has no latency (especially if there is already a test that exercises the code paths with latency). Consider just injecting latency for all of these tests? http://gerrit.cloudera.org:8080/#/c/17990/3/src/kudu/tserver/tablet_server-test.cc@805 PS3, Line 805: nit: adjust spacing to align the quotes http://gerrit.cloudera.org:8080/#/c/17990/3/src/kudu/tserver/tablet_server-test.cc@803 PS3, Line 803: void IsStatusValid(const string current_status) { : ASSERT_STR_MATCHES(current_status, "\"init_status\":[100|0]" : ".*\"read_filesystem_status\":[100|0]" : ".*\"read_instance_metadatafiles_status\":[100|0]" : ".*\"read_data_directories_status\":" : "([0-9]|[1-9][0-9]|100)" : ".*\"start_tablets_status\":([0-9]|[1-9][0-9]|100)" : ".*\"start_rpc_server_status\":[100|0]"); : } : : void IsStatusComplete(const string current_status) { : ASSERT_STR_MATCHES(current_status, "\"init_status\":100" : ".*\"read_filesystem_status\":100" : ".*\"read_instance_metadatafiles_status\":100" : ".*\"read_data_directories_status\":100" : ".*\"start_tablets_status\":100" : ".*\"start_rpc_server_status\":100"); : } nit: adjust spacing since these are in the scope of a class http://gerrit.cloudera.org:8080/#/c/17990/3/src/kudu/tserver/tablet_server-test.cc@901 PS3, Line 901: nit: remove the extra whitespace http://gerrit.cloudera.org:8080/#/c/17990/3/src/kudu/tserver/tablet_server-test.cc@926 PS3, Line 926: &bu nit: add a space, same below, and same in other places with the pattern ",&" http://gerrit.cloudera.org:8080/#/c/17990/3/src/kudu/tserver/tablet_server-test.cc@981 PS3, Line 981: ASSERT_OK(c.FetchURL( : Substitute("http://$0/metrics?metrics=log_block_manager_containers_processing_time_startup";, : addr),&buf)); : time_elapsed = buf.ToString(); : ASSERT_STR_NOT_CONTAINS(time_elapsed, "\"value\": 0"); : SleepFor(MonoDelta::FromMilliseconds(10)); : ASSERT_OK(c.FetchURL( : Substitute("http://$0/metrics?metrics=log_block_manager_containers_processing_time_startup";, : addr),&buf)); : ASSERT_EQ(time_elapsed, buf.ToString()); nit: this pattern seems to be used a lot. Consider defining a reusable method, e.g. void CheckNonZeroMetricIsStable(const string& metric_name) { const auto url = Substitute("http://$0/metrics?metrics=$1";, addr, metric_name); ASSERT_OK(c.FetchURL(url, &buf)); const auto orig_buf = buf.ToString(); ASSERT_STR_NOT_CONTAINS(orig_buf, "\"value\": 0"); SleepFor(MonoDelta::FromMilliseconds(10)); ASSERT_OK(c.FetchURL(url, &buf)); ASSERT_EQ(orig_buf, buf.ToString()); } -- 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: 3 Gerrit-Owner: Abhishek Chennaka <[email protected]> Gerrit-Reviewer: Abhishek Chennaka <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Wed, 03 Nov 2021 21:43:40 +0000 Gerrit-HasComments: Yes
