Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/9882 )
Change subject: [tools] ksck improvements [2/n]: KUDU-2306: ksck master health check ...................................................................... Patch Set 3: (10 comments) http://gerrit.cloudera.org:8080/#/c/9882/3/src/kudu/tools/ksck.h File src/kudu/tools/ksck.h: http://gerrit.cloudera.org:8080/#/c/9882/3/src/kudu/tools/ksck.h@240 PS3, Line 240: // Information has not yet been fetched. : UNINITIALIZED, : // The attempt to fetch information failed. : FETCH_FAILED, : // Information was fetched successfully. : FETCHED, Might be missing something, but it seems like we don't gain much from the extra expressiveness of UNINITIALIZED vs FETCH_FAILED. Couldn't we get by with some: bool fetch_succeeded; ...or something? http://gerrit.cloudera.org:8080/#/c/9882/3/src/kudu/tools/ksck.h@289 PS3, Line 289: masters nit: "uninitialized masters" or "masters from which we have not successfully fetched info" or something http://gerrit.cloudera.org:8080/#/c/9882/3/src/kudu/tools/ksck.h@338 PS3, Line 338: CHECK_NE(state_, KsckFetchState::UNINITIALIZED); nit: swap order here and elsewhere http://gerrit.cloudera.org:8080/#/c/9882/3/src/kudu/tools/ksck.h@533 PS3, Line 533: ServerHealthScore Doc this? http://gerrit.cloudera.org:8080/#/c/9882/3/src/kudu/tools/ksck.h@535 PS3, Line 535: Summarize nit: Summarizes http://gerrit.cloudera.org:8080/#/c/9882/3/src/kudu/tools/ksck.h@575 PS3, Line 575: static Status PrintServerHealthSummaries(const std::string& summary_type, Doc this. Also what are expected summary types? Maybe we should use an enum for that. Ah I see, maybe rename this to "server_type" or "server_name" or "daemon_type"? http://gerrit.cloudera.org:8080/#/c/9882/3/src/kudu/tools/ksck.cc File src/kudu/tools/ksck.cc: http://gerrit.cloudera.org:8080/#/c/9882/3/src/kudu/tools/ksck.cc@126 PS3, Line 126: std::ostream& operator<<(std::ostream& lhs, KsckFetchState state) { : switch (state) { : case KsckFetchState::UNINITIALIZED: : lhs << "UNINITIALIZED"; : break; : case KsckFetchState::FETCH_FAILED: : lhs << "FETCH_FAILED"; : break; : case KsckFetchState::FETCHED: : lhs << "FETCHED"; : break; : default: : LOG(FATAL) << "unknown KsckFetchState"; : } : return lhs; : } Hrmm I was going to suggest maybe a ToString() method, but I can't seem to find where this is being used. http://gerrit.cloudera.org:8080/#/c/9882/3/src/kudu/tools/ksck.cc@186 PS3, Line 186: if (!s.ok()) { : Warn() << Substitute("Unable to connect to master $0: $1", : master->ToString(), s.ToString()) << endl; : } : if (!s.ok()) bad_masters++; nit: merge these? http://gerrit.cloudera.org:8080/#/c/9882/3/src/kudu/tools/ksck_remote-test.cc File src/kudu/tools/ksck_remote-test.cc: http://gerrit.cloudera.org:8080/#/c/9882/3/src/kudu/tools/ksck_remote-test.cc@a217 PS3, Line 217: nit: probably belongs in CR [1/n]? http://gerrit.cloudera.org:8080/#/c/9882/3/src/kudu/tools/ksck_remote.cc File src/kudu/tools/ksck_remote.cc: http://gerrit.cloudera.org:8080/#/c/9882/3/src/kudu/tools/ksck_remote.cc@383 PS3, Line 383: std::static_pointer_cast<RemoteKsckMaster>(master)->Init(), What do you think about putting Init() into the KsckMaster base class? That way we can avoid this static casting? And maybe use auto instead of value_type? -- To view, visit http://gerrit.cloudera.org:8080/9882 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie0066468a0adc563979dcd2c61cd65d66978420c Gerrit-Change-Number: 9882 Gerrit-PatchSet: 3 Gerrit-Owner: Will Berkeley <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Reviewer: Will Berkeley <[email protected]> Gerrit-Comment-Date: Tue, 03 Apr 2018 05:17:37 +0000 Gerrit-HasComments: Yes
