Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/10288 )
Change subject: [tools] ksck improvements [7/n] Add JSON output option to ksck ...................................................................... Patch Set 8: (4 comments) http://gerrit.cloudera.org:8080/#/c/10288/8//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/10288/8//COMMIT_MSG@21 PS8, Line 21: https://gist.github.com/wdberkeley/0b4a4e38a9bd85acdaec627782217794 nit: Would be nice to see the other formats, even if they're not as readable. http://gerrit.cloudera.org:8080/#/c/10288/8/src/kudu/tools/ksck-test.cc File src/kudu/tools/ksck-test.cc: http://gerrit.cloudera.org:8080/#/c/10288/8/src/kudu/tools/ksck-test.cc@377 PS8, Line 377: CHECK nit: mind renaming these to EXPECT_* or VERIFY* or something? CHECK_* makes it seem like a mismatch would crash the test. Another nit: mind documenting what `reader`, `value`, and `field` are? `value` in particular, upon first glance I thought it was the same as `actual` below. Or maybe rename `value` to `json` or something http://gerrit.cloudera.org:8080/#/c/10288/8/src/kudu/tools/ksck_results.h File src/kudu/tools/ksck_results.h: http://gerrit.cloudera.org:8080/#/c/10288/8/src/kudu/tools/ksck_results.h@279 PS8, Line 279: // Print this KsckResults to 'out', according to the PrintMode 'mode'. : Status PrintTo(PrintMode mode, std::ostream& out); : : // Print this KsckResults to 'out' in json format. : // 'mode' must be PrintMode::JSON_PRETTY or PrintMode::JSON_COMPACT. : Status PrintJsonTo(std::ostream& out, PrintMode mode) const; nit: maybe keep the ordering the same? Also not the fault of this patch, but should PrintTo() be const? http://gerrit.cloudera.org:8080/#/c/10288/8/src/kudu/tools/ksck_results.h@318 PS8, Line 318: // Methods for writing components of KsckResults as json. : void ConsensusStateToJson(const KsckConsensusState& cstate, JsonWriter* w); : void MasterConsensusStatesToJson(const KsckConsensusStateMap& master_cstates, : bool conflict, : JsonWriter* w); : void ServerHealthSummariesToJson(KsckServerType type, : const std::vector<KsckServerHealthSummary>& summaries, : JsonWriter* w); : void TableSummariesToJson(const std::vector<KsckTableSummary>& table_summaries, : JsonWriter* w); : void TabletSummariesToJson(const std::vector<KsckTabletSummary>& tablet_summaries, : JsonWriter* w); : void ChecksumResultsToJson(const KsckChecksumResults& checksum_results, : JsonWriter* w); : void TotalCountsToJson(const KsckResults& results, : JsonWriter* w); I might be missing something. Are these somewhere? I can't find them in ksck_results.cc -- To view, visit http://gerrit.cloudera.org:8080/10288 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib5da0752f8e41c022611253c300450368f6ae969 Gerrit-Change-Number: 10288 Gerrit-PatchSet: 8 Gerrit-Owner: Will Berkeley <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Will Berkeley <[email protected]> Gerrit-Comment-Date: Wed, 09 May 2018 00:01:58 +0000 Gerrit-HasComments: Yes
