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: (10 comments) http://gerrit.cloudera.org:8080/#/c/10288/7/src/kudu/tools/ksck-test.cc File src/kudu/tools/ksck-test.cc: http://gerrit.cloudera.org:8080/#/c/10288/7/src/kudu/tools/ksck-test.cc@478 PS7, Line 478: CHECK_JSON_STRING_FIELD(r, non_voter_uuids[j], nullptr, ref_non_voter_uuids[j]); : } : } I'm not sure I understand this couple of lines: look like a typo. 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."); > Are you saying you would like the default to be the compact json, so the js Maybe, it's worth renaming 'pretty' to 'text'? Also, it would be nice if it were explicitly said that 'json' and 'compact_json' are different just in formatting, but contain the same information. UPDATE: it seems you already renamed that into 'plain_xxx' and alike. So, my only suggestion here is to mention that 'json' and 'json_compact' (or 'json_pretty' and 'json_compact') contain the same information, but differ just in formatting. 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 > I chose COMPACT_JSON to be consistent with JsonWriter::Mode, which uses PRE OK, that sounds reasonable. If you want that to follow the same convention as JsonWriter::Mode, that's fine with me. I'm then fine if you leave those names as is. In that case, maybe just add corresponding comment, since it seems I was confused by the comment for the JSON format that says 'Includes all information', but I didn't see a notion on 'all information' in the COMPACT_JSON section. Basically, if I understood correctly, the different between JSON and JSON_COMPACT is just formatting difference. If so, it would be nice if it were said explicitly in the comments. Thanks! http://gerrit.cloudera.org:8080/#/c/10288/3/src/kudu/tools/ksck_results.cc File src/kudu/tools/ksck_results.cc: http://gerrit.cloudera.org:8080/#/c/10288/3/src/kudu/tools/ksck_results.cc@634 PS3, Line 634: out<< JsonWriter::ToJson(pb, jw_mode) << endl; nit: add space between 'out' and '<<' http://gerrit.cloudera.org:8080/#/c/10288/3/src/kudu/tools/tool.proto File src/kudu/tools/tool.proto: http://gerrit.cloudera.org:8080/#/c/10288/3/src/kudu/tools/tool.proto@218 PS3, Line 218: HEALTHY = 0; nit: since the 'health' field is optional, if it's not set then it should be read as HEALTHY if not checking first with has_health(). Is the the intended behavior? In other words, what do you think setting UNKNOWN = 0 and having it as the first field in the enumeration? http://gerrit.cloudera.org:8080/#/c/10288/3/src/kudu/tools/tool.proto@234 PS3, Line 234: UNKNOWN = 999 ditto http://gerrit.cloudera.org:8080/#/c/10288/3/src/kudu/tools/tool.proto@250 PS3, Line 250: UNKNOWN = 999; ditto http://gerrit.cloudera.org:8080/#/c/10288/3/src/kudu/tools/tool.proto@254 PS3, Line 254: extra spaces http://gerrit.cloudera.org:8080/#/c/10288/7/src/kudu/tools/tool.proto File src/kudu/tools/tool.proto: http://gerrit.cloudera.org:8080/#/c/10288/7/src/kudu/tools/tool.proto@231 PS7, Line 231: MASTER = 0; Can masters have pending config for their system tablet? If yes, then how to distinguish that config from a committed one? http://gerrit.cloudera.org:8080/#/c/10288/3/src/kudu/tools/tool_action_cluster.cc File src/kudu/tools/tool_action_cluster.cc: http://gerrit.cloudera.org:8080/#/c/10288/3/src/kudu/tools/tool_action_cluster.cc@106 PS3, Line 106: ksck_format Maybe, just 'format', 'out_format' or 'output_format'? It's already ksck sub-command, right? -- 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 04:28:49 +0000 Gerrit-HasComments: Yes
