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 9: (9 comments) 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 Done 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? Done 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 Done http://gerrit.cloudera.org:8080/#/c/10288/9/src/kudu/tools/ksck-test.cc@595 PS9, Line 595: " > nit: spacing Done http://gerrit.cloudera.org:8080/#/c/10288/9/src/kudu/tools/ksck-test.cc@626 PS9, Line 626: " > nit: spacing Done 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 C Done 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 messag See the "Yowza!" comment. 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. It's forced by the way protobuf stringifies its enum names. We have to match it here or do gymnastics just to keep it the same. I'll call it out in the commit msg. 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 Done -- 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:22:17 +0000 Gerrit-HasComments: Yes
