Abhishek Chennaka 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: (11 comments) http://gerrit.cloudera.org:8080/#/c/17990/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/17990/3//COMMIT_MSG@7 PS3, Line 7: KUDU-1959 - Implement tests for the startup progress page for tablet servers > This is 76 characters which seems a bit too long. Generally, we wrap lines Done http://gerrit.cloudera.org:8080/#/c/17990/3/src/kudu/tserver/tablet_server-test.cc File src/kudu/tserver/tablet_server-test.cc: PS3: > Some of the concerns raised in the parent apply here as well. Got it. http://gerrit.cloudera.org:8080/#/c/17990/3/src/kudu/tserver/tablet_server-test.cc@31 PS3, Line 31: > nit: remove the extra newline Done http://gerrit.cloudera.org:8080/#/c/17990/3/src/kudu/tserver/tablet_server-test.cc@46 PS3, Line 46: > nit: remove the extra newlines Done 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 (espec Makes sense. Done http://gerrit.cloudera.org:8080/#/c/17990/3/src/kudu/tserver/tablet_server-test.cc@784 PS3, Line 784: /*num_data_dirs=*/ > nit: no need for this as you're using a constant which describes what this Done 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 Done 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 Done http://gerrit.cloudera.org:8080/#/c/17990/3/src/kudu/tserver/tablet_server-test.cc@901 PS3, Line 901: > nit: remove the extra whitespace Done 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 ",& Done 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 meth 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: 3 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 03:40:19 +0000 Gerrit-HasComments: Yes
