Will Berkeley 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 4: (9 comments) http://gerrit.cloudera.org:8080/#/c/10061/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/10061/1//COMMIT_MSG@22 PS1, Line 22: are two rows : for the > Mmm I had the same thought which inspired the flag to turn off the flags ch Done http://gerrit.cloudera.org:8080/#/c/10061/4/src/kudu/tools/ksck.h File src/kudu/tools/ksck.h: http://gerrit.cloudera.org:8080/#/c/10061/4/src/kudu/tools/ksck.h@356 PS4, Line 356: state_, KsckFetchState::UNINITIALIZED > nit: reverse the order of these here and L252 Done http://gerrit.cloudera.org:8080/#/c/10061/4/src/kudu/tools/ksck.h@356 PS4, Line 356: CHECK_NE(state_, KsckFetchState::UNINITIALIZED); > Does state_ reflect all of FetchUnusualFlags, FetchConsensusState, and Fetc state_ should reflect the tripartite state of fetching the core info from the server, so we shouldn't touch it in the flags stuff since it's extra. Just for sanity checks I can add another state for flags fetching. http://gerrit.cloudera.org:8080/#/c/10061/4/src/kudu/tools/ksck.h@485 PS4, Line 485: Check for "unusual" flags on masters. : // "Unusual" flags are ones tagged hidden, experimental, or unsafe. > Note only non-default unusual flags. I don't think that's mentioned anywher Done http://gerrit.cloudera.org:8080/#/c/10061/4/src/kudu/tools/ksck.cc File src/kudu/tools/ksck.cc: http://gerrit.cloudera.org:8080/#/c/10061/4/src/kudu/tools/ksck.cc@298 PS4, Line 298: "Some masters have unsafe, experimental, or hidden flags set")); > nit: spacing Done http://gerrit.cloudera.org:8080/#/c/10061/4/src/kudu/tools/ksck.cc@302 PS4, Line 302: return Status::Incomplete( > nit: spacing Done http://gerrit.cloudera.org:8080/#/c/10061/4/src/kudu/tools/ksck.cc@436 PS4, Line 436: "Some tablet servers have unsafe, experimental, or hidden flags set")); > nit: spacing Done 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; > Can we put this down in TestCheckUnusualFlags? It's kind of weird seeing L2 We should set it here in SetUp before we start the minicluster. I will make a note that we set an unusual flag in the test itself. I could also subclass RemoteKsckTest but that feels like overkill. http://gerrit.cloudera.org:8080/#/c/10061/4/src/kudu/tools/ksck_remote-test.cc@229 PS4, Line 229: ASSERT_OK(ksck_->CheckMasterHealth()); : ASSERT_OK(ksck_->CheckMasterHealth()); : ASSERT_OK(ksck_->CheckMasterUnusualFlags()); : ASSERT_OK(ksck_->CheckMasterConsensus()); : ASSERT_OK(ksck_->CheckClusterRunning()); : ASSERT_OK(ksck_->FetchTableAndTabletInfo()); : ASSERT_OK(ksck_->FetchInfoFromTabletServers()); : ASSERT_OK(ksck_->CheckTabletServerUnusualFlags()); > Must all of these be run to pass this test? Weeeeell there was an extra CheckMasterHealth in there, and CheckMasterConsensus isn't needed, but otherwise yes. CheckMasterHealth gathers the info. CheckMasterUnusualFlags does the master flag check The next 3 do the info gathering for tservers. CheckTabletServerUnusualFlags does the tserver flag check. -- 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: 4 Gerrit-Owner: Will Berkeley <wdberke...@gmail.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Attila Bukor <abu...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-Reviewer: Will Berkeley <wdberke...@gmail.com> Gerrit-Comment-Date: Fri, 25 May 2018 07:19:46 +0000 Gerrit-HasComments: Yes
