Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/10151 )
Change subject: [WIP] [tools] ksck improvements [6/n]: Refactor printing ...................................................................... Patch Set 5: (9 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? Also, maybe it's separating that change in a stand-alone changelist? 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: 1. Run() 2. PrintResults() ? 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: Status s = RunKsck(); : : // Every table we checked was healthy ;). : ASSERT_OK(s); Here an in other places: ASSERT_OK(RunKsck()) http://gerrit.cloudera.org:8080/#/c/10151/5/src/kudu/tools/ksck-test.cc@858 PS5, Line 858: Status s = RunKsck(); : : ASSERT_OK(s); nit: why not just ASSERT_OK(RunKsck()) ? http://gerrit.cloudera.org:8080/#/c/10151/5/src/kudu/tools/ksck-test.cc@878 PS5, Line 878: Status s = RunKsck(); : : ASSERT_OK(s); ASSERT_OK(RunKsck()) ? 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? http://gerrit.cloudera.org:8080/#/c/10151/5/src/kudu/tools/ksck_results.h@126 PS5, Line 126: // Returns an int signifying the "unhealthiness level" of 'sh'. : // Useful for sorting or comparing. It would be nice to mention what higher score value means, i.e. if server A has score 10 and server B has score 20, which server is 'healthier'? http://gerrit.cloudera.org:8080/#/c/10151/5/src/kudu/tools/ksck_results.h@133 PS5, Line 133: address What if server has multiple RPC endpoints? http://gerrit.cloudera.org:8080/#/c/10151/5/src/kudu/tools/ksck_results.h@257 PS5, Line 257: const std::vector<KsckServerHealthSummary> nit: why not constant reference? -- 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: 5 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: Tue, 24 Apr 2018 22:39:27 +0000 Gerrit-HasComments: Yes
