Andrew Wong 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 9: (9 comments) Mostly nits, looks good! http://gerrit.cloudera.org:8080/#/c/10288/9/src/kudu/tools/ksck-test.cc File src/kudu/tools/ksck-test.cc: http://gerrit.cloudera.org:8080/#/c/10288/9/src/kudu/tools/ksck-test.cc@453 PS9, Line 453: K nit: spacing http://gerrit.cloudera.org:8080/#/c/10288/9/src/kudu/tools/ksck-test.cc@489 PS9, Line 489: EXTRACT_ARRAY_CHECK_SIZE(r, cstate, "nonvoter_uuids", : non_voter_uuids, ref_non_voter_uuids.size()); Is there a reason to not do the EXPECT_JSON_STRING_FIELD checks here? http://gerrit.cloudera.org:8080/#/c/10288/9/src/kudu/tools/ksck-test.cc@561 PS9, Line 561: "health", KsckCheckResultToString(ref_table.TableStatus())); : EXPECT_JSON_INT_FIELD(r, table, : "replication_factor", ref_table.replication_factor); : EXPECT_JSON_INT_FIELD(r, table, : "total_tablets", ref_table.TotalTablets()); : EXPECT_JSON_INT_FIELD(r, table, : "healthy_tablets", ref_table.healthy_tablets); : EXPECT_JSON_INT_FIELD(r, table, : "recovering_tablets", ref_table.recovering_tablets); : EXPECT_JSON_INT_FIELD(r, table, : "underreplicated_tablets", ref_table.underreplicated_tablets); : EXPECT_JSON_INT_FIELD(r, table, : "unavailable_tablets", ref_table.unavailable_tablets); : EXPECT_JSON_INT_FIELD(r, table, : "consensus_mismatch_tablets", ref_table.consensus_mismatch_tablets); nit: spacing http://gerrit.cloudera.org:8080/#/c/10288/9/src/kudu/tools/ksck-test.cc@595 PS9, Line 595: " nit: spacing http://gerrit.cloudera.org:8080/#/c/10288/9/src/kudu/tools/ksck-test.cc@626 PS9, Line 626: " nit: spacing http://gerrit.cloudera.org:8080/#/c/10288/9/src/kudu/tools/ksck-test.cc@693 PS9, Line 693: std::transform(std::begin(results.error_messages), : std::end(results.error_messages), : std::back_inserter(errors), : [](const Status& s) { return s.ToString(); }); : CheckJsonVsErrors(r, "errors", errors); nit: you could do away with this transform by passing `vector<Status>` to CheckJsonVsErrors and doing ref_errors[i].ToString() up at L676. Then we could do away with <algorithm> http://gerrit.cloudera.org:8080/#/c/10288/9/src/kudu/tools/ksck-test.cc@1046 PS9, Line 1046: /*consensus_mismatch_tablets=*/ 1, : /*unavailable_tablets=*/ 0)); nit: whatever fixed this is probably worth calling out in the commit message. http://gerrit.cloudera.org:8080/#/c/10288/9/src/kudu/tools/ksck_results.cc File src/kudu/tools/ksck_results.cc: http://gerrit.cloudera.org:8080/#/c/10288/9/src/kudu/tools/ksck_results.cc@133 PS9, Line 133: return "CONSENSUS_MISMATCH"; Yowza! I wonder if this should go in a separate patch. http://gerrit.cloudera.org:8080/#/c/10288/9/src/kudu/tools/tool_action_cluster.cc File src/kudu/tools/tool_action_cluster.cc: http://gerrit.cloudera.org:8080/#/c/10288/9/src/kudu/tools/tool_action_cluster.cc@106 PS9, Line 106: .AddOptionalParameter("ksck_format") nit: sort order -- 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: 9 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 23:02:33 +0000 Gerrit-HasComments: Yes
