Will Berkeley 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 readabl Done 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 Done. I'm keeping the name 'value' though, since it's the rapidjson::Value* from which the field value will be extracted and that name helped me remember the order when I was writing the code. 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, bu Done. Ideally PrintTo() would be const but we sort various pieces of information in order to pretty-print it. I opted to make it non-const instead of making lots of copies. 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 ksc Done -- 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 <wdberke...@gmail.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Will Berkeley <wdberke...@gmail.com> Gerrit-Comment-Date: Thu, 10 May 2018 18:57:19 +0000 Gerrit-HasComments: Yes
