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 3: (3 comments) Since we're bikeshedding I changed the ksck_format and PrintMode bits a bit. Let me know what you think. 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 Done 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 we Are you saying you would like the default to be the compact json, so the json format flag values would be 'json' and 'pretty_json'? The reason I chose 'json' and 'compact_json' is just so it's shorter and easier to type the more common value when playing around on the command line. 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 i I chose COMPACT_JSON to be consistent with JsonWriter::Mode, which uses PRETTY and COMPACT. It makes sense to change to JSON_PRETTY and JSON_COMPACT, but I object to JSON_VERBOSE because verbose implies more information, when in fact pretty and compact json have exactly the same info, they just differ in formatting. -- 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: Sat, 05 May 2018 02:53:11 +0000 Gerrit-HasComments: Yes
