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 7: (7 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`? Whoops, meant = 1. 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 Well, it's actually intentional, but if you don't like it that much... 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 Done 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 agree with Attila, it'd be weird for programmers and code reviewers in th Mmm I see what you guys mean, it works because we are using an internal minicluster. Sorry I was thick and didn't figure that out before. 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? I thought not but actually the test will usually fail if this is replaced with ksck_->Run(). The reason is that calling CheckTablesConsistency will frequently find at least one tablet in the state where it has a leader but the leader hasn't asserted its leadership yet, so the other nodes think there's no leader. It's easy to solve by just inserting some rows at the start of the test, and we ought to actually do that in each of these tests as its the simplest way to wait until the tables are fully ready. Then we can use ksck_->RunAndPrintResults() everywhere. Doing that involves changing every test and doing a workaround for some checksum tests as they insert N rows and expect to checksum N rows, so if we insert rows in the test fixture we need to expect those in the checksum tests. Is it ok if I do a quick followup for that stuff after this patch? 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? The block L144-146 guarantees that n > server.size() here. http://gerrit.cloudera.org:8080/#/c/10061/7/src/kudu/tools/ksck_results.cc@374 PS7, Line 374: const string& name = flag.first.first; : const string& value = flag.first.second; : flags_table.AddRow({name, : value, : FindOrDie(flag_tags_map, name), : ServerCsv(num_servers, flag.second)}); : } > nit: spacing Done -- 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 21:36:11 +0000 Gerrit-HasComments: Yes
