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 7: (9 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: EXTRACT_ARRAY_CHECK_SIZE(r, cstate, "nonvoter_uuids", : non_voter_uuids, ref_non_voter_uuids.size()); : CHECK_JSON_FIELD_NOT_PRESENT(r, cstate, "non_voter_uuids"); > I'm not sure I understand this couple of lines: look like a typo. 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, "plain_concise", : "Output format for ksck. Available options are 'plain_concise', " : "'plain_full', 'json_pretty', and 'json_compact'.\n" : "'plain_concise' format is plain text, omitting most information " : "about healthy tablets.\n" > Maybe, it's worth renaming 'pretty' to 'text'? Done 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: PLAIN_FULL, > OK, that sounds reasonable. If you want that to follow the same convention Done 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: return Status::OK(); > nit: add space between 'out' and '<<' Done 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; Yes, I didn't do this right. Our standard is to have UNKNOWN at the top with value 99. That's because, from https://developers.google.com/protocol-buffers/docs/proto#optional, > For enums, the default value is the first value listed in the enum's type > definition. http://gerrit.cloudera.org:8080/#/c/10288/3/src/kudu/tools/tool.proto@250 PS3, Line 250: UNKNOWN = 999; > ditto Done http://gerrit.cloudera.org:8080/#/c/10288/3/src/kudu/tools/tool.proto@254 PS3, Line 254: > extra spaces Done 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 Perhaps the name is not the best? A MASTER ConfigType means it's the cstate the master has for that tablet; it got it from the last leader. It doesn't apply to the master tablet because there's no master of the masters. It's always COMMITTED since only committed info is reported to the master. Also, there's no such thing as a pending config for the master tablet. We do not presently allow config changes for the master tablet. So all configs of the master tablet are COMMITTED. 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 s 'format' is taken, and in fact affects the format of ksck output in plain text modes since it changes how DataTables are printed. 'output_format' is the next obvious choice, but it's the obvious choice for every new formatting flag that would like to be called 'format' as well. 'ksck_format' effectively namespaces the flag, and distinguishes it from 'format' because it affects the format of the entire output. It'd be nice if 'format' could be called 'table_format'. Not sure if people think it's too late for that. -- 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: 7 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 09:17:54 +0000 Gerrit-HasComments: Yes
