Abhishek Chennaka has posted comments on this change. ( http://gerrit.cloudera.org:8080/17915 )
Change subject: KUDU-1959 - Implement tests for the startup web page for Kudu servers ...................................................................... Patch Set 1: (6 comments) All done and abandoning this patch as this is split now http://gerrit.cloudera.org:8080/#/c/17915/1/src/kudu/tserver/tablet_server-test.cc File src/kudu/tserver/tablet_server-test.cc: http://gerrit.cloudera.org:8080/#/c/17915/1/src/kudu/tserver/tablet_server-test.cc@803 PS1, Line 803: bool > This will be racy, so we should use an atomic<bool> Done http://gerrit.cloudera.org:8080/#/c/17915/1/src/kudu/tserver/tablet_server-test.cc@811 PS1, Line 811: continue; > nit: maybe sleep here too so we're not a tight loop while starting up. Same Done http://gerrit.cloudera.org:8080/#/c/17915/1/src/kudu/tserver/tablet_server-test.cc@827 PS1, Line 827: { : is_started = true; : read_startup_page.join(); : }); > nit: in our codebase this is usually formatted as Done http://gerrit.cloudera.org:8080/#/c/17915/1/src/kudu/tserver/tablet_server-test.cc@846 PS1, Line 846: // Fail a data directory. : FLAGS_env_inject_eio_globs = fs_manager->GetDataRootDirs()[1]; : FLAGS_env_inject_eio = 1.0; > nit: it's probably worth moving this down to right below calling Start() Done, and the same for the "FLAGS_tablet_bootstrap_inject_latency_ms" in the above test. http://gerrit.cloudera.org:8080/#/c/17915/1/src/kudu/tserver/tablet_server-test.cc@862 PS1, Line 862: ASSERT_STR_MATCHES(thread_buf.ToString(), "\"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]"); > nit: maybe define this in an anonymous namespace as a helper function that Done http://gerrit.cloudera.org:8080/#/c/17915/1/src/kudu/tserver/tablet_server-test.cc@890 PS1, Line 890: INSTANTIATE_TEST_SUITE_P(Params, TabletServerStartupWebPageTest, : ::testing::ValuesIn(BlockManager::block_manager_types())); > Given the above tests are near identical, consider using ::testing::Combine Guess it makes more sense to cross test block manager types with whether we're injecting tablet bootstrap latency. This approach seems to cover all the cases. -- To view, visit http://gerrit.cloudera.org:8080/17915 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I80880ecbda6a9f6db85baaf109af7e701111328d Gerrit-Change-Number: 17915 Gerrit-PatchSet: 1 Gerrit-Owner: Abhishek Chennaka <[email protected]> Gerrit-Reviewer: Abhishek Chennaka <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Tue, 02 Nov 2021 05:27:56 +0000 Gerrit-HasComments: Yes
