Alexey Serbin 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 3: (3 comments) Thank you for getting this closer to be machine-readable state! I just skimmed through, bike-shedding. I'm planning to take a closer look this evening. http://gerrit.cloudera.org:8080/#/c/10288/3/src/kudu/tools/ksck-test.cc File src/kudu/tools/ksck-test.cc: http://gerrit.cloudera.org:8080/#/c/10288/3/src/kudu/tools/ksck-test.cc@375 PS3, Line 375: EXPECT_EQ(actual, (expected)); It's better to have the expected value to come first -- that way the error messages are much easier to comprehend, if any failure happens. http://gerrit.cloudera.org:8080/#/c/10288/3/src/kudu/tools/ksck.cc File src/kudu/tools/ksck.cc: http://gerrit.cloudera.org:8080/#/c/10288/3/src/kudu/tools/ksck.cc@75 PS3, Line 75: DEFINE_string(ksck_format, "pretty", : "Output format for ksck. Available options are 'pretty', 'json'," : "and 'compact_json'. 'pretty' format is organized so it's easy for " : "humans to read and interpret. 'json' produces pretty-printed " : "json; 'compact_json' is suitable for parsing by other programs."); I would expect 'pretty' to be a modifier for any format, and for JSON as well (e.g., the pretty-formatted JSON has the structure that is easier for humans to read). http://gerrit.cloudera.org:8080/#/c/10288/3/src/kudu/tools/ksck_results.h File src/kudu/tools/ksck_results.h: http://gerrit.cloudera.org:8080/#/c/10288/3/src/kudu/tools/ksck_results.h@249 PS3, Line 249: COMPACT_JSON naming nit: given the name of the field, I would expect that COMPACT_JSON is just JSON but without proper formatting. Maybe, name it something like JSON_TERSE or maybe name this JSON, but rename JSON into JSON_VERBOSE ? -- 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: 3 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: Fri, 04 May 2018 19:56:18 +0000 Gerrit-HasComments: Yes
