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 7: (6 comments) http://gerrit.cloudera.org:8080/#/c/10061/7//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/10061/7//COMMIT_MSG@37 PS7, Line 37: localhost:7052 and 1 other server(s) Shouldn't this be not truncated if `--truncate_server_csv_length=2`? http://gerrit.cloudera.org:8080/#/c/10061/7/src/kudu/tools/ksck.cc File src/kudu/tools/ksck.cc: http://gerrit.cloudera.org:8080/#/c/10061/7/src/kudu/tools/ksck.cc@202 PS7, Line 202: }); : : sh nit: extra line here http://gerrit.cloudera.org:8080/#/c/10061/7/src/kudu/tools/ksck.cc@349 PS7, Line 349: if (FLAGS_consensus) { : return ts->FetchConsensusState(&health); : } : return Status::OK(); : }); nit: revert here too http://gerrit.cloudera.org:8080/#/c/10061/5/src/kudu/tools/ksck_remote-test.cc File src/kudu/tools/ksck_remote-test.cc: http://gerrit.cloudera.org:8080/#/c/10061/5/src/kudu/tools/ksck_remote-test.cc@95 PS5, Line 95: > I meant adding this flag only in the test context, but I'm not sure if it's I agree with Attila, it'd be weird for programmers and code reviewers in the future to have to spend cycles on an update to this test for an update of the flag. http://gerrit.cloudera.org:8080/#/c/10061/7/src/kudu/tools/ksck_remote-test.cc File src/kudu/tools/ksck_remote-test.cc: http://gerrit.cloudera.org:8080/#/c/10061/7/src/kudu/tools/ksck_remote-test.cc@216 PS7, Line 216: 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()); nit: any reason to not use ksck_->Run() here? No checksumming? http://gerrit.cloudera.org:8080/#/c/10061/7/src/kudu/tools/ksck_results.cc File src/kudu/tools/ksck_results.cc: http://gerrit.cloudera.org:8080/#/c/10061/7/src/kudu/tools/ksck_results.cc@149 PS7, Line 149: n Do you think it's worth validating this is positive? -- 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: 7 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 20:16:41 +0000 Gerrit-HasComments: Yes
