Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/10151 )
Change subject: [WIP] [tools] ksck improvements [6/n]: Refactor printing ...................................................................... Patch Set 6: (15 comments) http://gerrit.cloudera.org:8080/#/c/10151/5//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/10151/5//COMMIT_MSG@17 PS5, Line 17: - Removes the --consensus flag. ksck is far less useful without it and : it doesn't seem like having it brings any benefit. > Any concerns about that in terms of backward compatibility? I don't think we make guarantees on backwards compat for tools. Also, I don't think anyone besides me ever used that flag. The good info comes from the consensus state :) I see the point about removing it separately but if I make it a patch prior to this one I'm spending time working in the "old way" and have to rejigger the patch to match those changes. Seems wasteful. If I make it a patch after this one I have to redo pieces of this patch to re-introduce it, just so I can take it out. Ergo, unless you have a big problem with keeping it part of this patch, I'd rather leave it in. http://gerrit.cloudera.org:8080/#/c/10151/5/src/kudu/integration-tests/cluster_verifier.cc File src/kudu/integration-tests/cluster_verifier.cc: http://gerrit.cloudera.org:8080/#/c/10151/5/src/kudu/integration-tests/cluster_verifier.cc@112 PS5, Line 112: RunAndPrintResults > Would it make sense to separate that in two methods: Done http://gerrit.cloudera.org:8080/#/c/10151/5/src/kudu/tools/ksck-test.cc File src/kudu/tools/ksck-test.cc: http://gerrit.cloudera.org:8080/#/c/10151/5/src/kudu/tools/ksck-test.cc@845 PS5, Line 845: /*underreplicated_tables=*/ 3, : /*consensus_mismatch_tables=*/ 0, : /*unavailable_tables=*/ 0)); : } > Here an in other places: Done. Plus some changes due to how I wrote RunAndPrintResults(). http://gerrit.cloudera.org:8080/#/c/10151/5/src/kudu/tools/ksck-test.cc@858 PS5, Line 858: // Now engineer a consensus conflict. : const shared_ptr<KsckTabletServer>& ts = FindOrDie(cluster_->tablet_servers_, "ts-id-1"); : auto& cstate > nit: why not just Done http://gerrit.cloudera.org:8080/#/c/10151/5/src/kudu/tools/ksck-test.cc@878 PS5, Line 878: ASSERT_STR_CONTAINS(err_stream_.str(), : ExpectedKsckTableSummary("test", : > ASSERT_OK(RunKsck()) ? Done http://gerrit.cloudera.org:8080/#/c/10151/5/src/kudu/tools/ksck_results.h File src/kudu/tools/ksck_results.h: http://gerrit.cloudera.org:8080/#/c/10151/5/src/kudu/tools/ksck_results.h@31 PS5, Line 31: "kudu/util/status.h" > why angle brackets instead of quotes? Oops. http://gerrit.cloudera.org:8080/#/c/10151/5/src/kudu/tools/ksck_results.h@126 PS5, Line 126: const char* const ServerHealthToString(KsckServerHealth sh); : > It would be nice to mention what higher score value means, i.e. if server A Done http://gerrit.cloudera.org:8080/#/c/10151/5/src/kudu/tools/ksck_results.h@133 PS5, Line 133: f a ser > What if server has multiple RPC endpoints? AFAIK we routinely don't handle this situation in our code. I don't think we support it overall even though some places are set up for it. What would end up here is what the master reports as the RPC endpoint. http://gerrit.cloudera.org:8080/#/c/10151/5/src/kudu/tools/ksck_results.h@257 PS5, Line 257: ch table and tablet. > nit: why not constant reference? Oops. http://gerrit.cloudera.org:8080/#/c/10151/5/src/kudu/tools/ksck_results.cc File src/kudu/tools/ksck_results.cc: http://gerrit.cloudera.org:8080/#/c/10151/5/src/kudu/tools/ksck_results.cc@88 PS5, Line 88: CHECK(uuid_label_mapping); > static? It seems this function can be entered multiple times during kudu C First point: It's in an anonymous namespace, so it's functionally equivalent to a static function. Second: Done. http://gerrit.cloudera.org:8080/#/c/10151/5/src/kudu/tools/ksck_results.cc@124 PS5, Line 124: switch (cr) { > nit: maybe, return 'const char* const' instead? Done. Also done for function above. http://gerrit.cloudera.org:8080/#/c/10151/5/src/kudu/tools/ksck_results.cc@177 PS5, Line 177: // First, report on the masters and master tablet. > nit: add a stop Done http://gerrit.cloudera.org:8080/#/c/10151/5/src/kudu/tools/ksck_results.cc@268 PS5, Line 268: } > nit: two extra spaces Done http://gerrit.cloudera.org:8080/#/c/10151/5/src/kudu/tools/ksck_results.cc@269 PS5, Line 269: uid : server_uuids) { : const string label(1, FindOrDie(replica_labels, uuid)); > consider adding replication factor, as it's done in https://gerrit.cloudera Done http://gerrit.cloudera.org:8080/#/c/10151/5/src/kudu/tools/ksck_results.cc@273 PS5, Line 273: continue; : } : const auto& cstate = FindOrDie(cstates, uuid); : AddRowForCState(label, cstate, replica_labels, &cmatrix); : } : out << "The consensus matrix is:" << endl; : return cmatrix.PrintTo(out); : } : : Status PrintServerHealthSummaries(KsckServerType type, : const vector<KsckServerHealthSummary>& summaries, : ostream& out) { : out << ServerTypeToString(type) << " Summary" << endl; : Dat > maybe, separate this into a function? Done -- To view, visit http://gerrit.cloudera.org:8080/10151 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id8de619996b6cd753e6a9c01b1b60810a873e609 Gerrit-Change-Number: 10151 Gerrit-PatchSet: 6 Gerrit-Owner: Will Berkeley <wdberke...@gmail.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Will Berkeley <wdberke...@gmail.com> Gerrit-Comment-Date: Wed, 25 Apr 2018 05:55:45 +0000 Gerrit-HasComments: Yes
