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 4: (8 comments) 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: CHECK_NE(state_, KsckFetchState::UNINITIALIZED); Does state_ reflect all of FetchUnusualFlags, FetchConsensusState, and FetchInfo? It's probably worth documenting its semantics, e.g. if FetchInfo() succeeds but FetchUnusualFlags() hasn't been called, is it still valid to call flags()? Is that an important point to make? Since we don't set the state in FetchUnusualFlags(), it seems a little weird to have this check here. 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 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 anywhere except the commit message. 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 http://gerrit.cloudera.org:8080/#/c/10061/4/src/kudu/tools/ksck.cc@302 PS4, Line 302: return Status::Incomplete( nit: spacing 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 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 L244 without seeing this. 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? -- 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 <[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 02:49:24 +0000 Gerrit-HasComments: Yes
