Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/10061 )
Change subject: [tools] ksck improvements [5/n]: Checks for experimental, unsafe, hidden flags ...................................................................... Patch Set 5: (5 comments) http://gerrit.cloudera.org:8080/#/c/10061/5//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/10061/5//COMMIT_MSG@30 PS5, Line 30: in a new warnings section near the : end of the ksck output. nit: mind including this in the above snippet too? http://gerrit.cloudera.org:8080/#/c/10061/5/src/kudu/tools/ksck.cc File src/kudu/tools/ksck.cc: http://gerrit.cloudera.org:8080/#/c/10061/5/src/kudu/tools/ksck.cc@200 PS5, Line 200: tatus s = master->FetchInfo() : .AndThen([&]() { : return master->FetchConsensusState(); : }); nit: revert formatting? http://gerrit.cloudera.org:8080/#/c/10061/4/src/kudu/tools/ksck_remote-test.cc File src/kudu/tools/ksck_remote-test.cc: http://gerrit.cloudera.org:8080/#/c/10061/4/src/kudu/tools/ksck_remote-test.cc@94 PS4, Line 94: // Add a harmless experimental flag so we can detect it with ksck. : FLAGS_safe_time_max_lag_ms = 60000; > We should set it here in SetUp before we start the minicluster. I will make Oh huh. I thought IMCs reflected all in-process updates of flags. Those don't show up in the flags endpoint? http://gerrit.cloudera.org:8080/#/c/10061/4/src/kudu/tools/ksck_remote-test.cc@229 PS4, Line 229: TEST_F(RemoteKsckTest, TestCheckUnusualFlags) { : ASSERT_OK(ksck_->CheckMasterHealth()); : ASSERT_OK(ksck_->CheckMasterUnusualFlags()); : ASSERT_OK(ksck_->CheckClusterRunning()); : ASSERT_OK(ksck_->FetchTableAndTabletInfo()); : ASSERT_OK(ksck_->FetchInfoFromTabletServers()); : ASSERT_OK(ksck_->CheckTabletServerUnusualFlags()); : ASSERT_OK(ksck_->PrintResults()); > Weeeeell there was an extra CheckMasterHealth in there, and CheckMasterCons Ah, I would've thought CheckMasterUnusualFlags() and CheckTabletServerUnusualFlags() were standalone. If that's not the case, mind documenting the prerequisites to running them? http://gerrit.cloudera.org:8080/#/c/10061/5/src/kudu/tools/ksck_results.cc File src/kudu/tools/ksck_results.cc: http://gerrit.cloudera.org:8080/#/c/10061/5/src/kudu/tools/ksck_results.cc@267 PS5, Line 267: s.ToString() Since these are warnings, let's just print `s.message()`. That way we won't confuse users by presenting an error type. -- To view, visit http://gerrit.cloudera.org:8080/10061 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Idd6c179e5256b2f2bae2f7486c5e0365ef184706 Gerrit-Change-Number: 10061 Gerrit-PatchSet: 5 Gerrit-Owner: Will Berkeley <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Reviewer: Will Berkeley <[email protected]> Gerrit-Comment-Date: Fri, 25 May 2018 16:08:03 +0000 Gerrit-HasComments: Yes
