Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/10535 )
Change subject: Fix ksck checksum_scan printing ...................................................................... Patch Set 6: (8 comments) http://gerrit.cloudera.org:8080/#/c/10535/6//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/10535/6//COMMIT_MSG@15 PS6, Line 15: kudu cluster ksck --checksum_scan --checksum_snapshot=false : --ksck_format=json_compact localhost:7052,localhost:7051, : localhost:7053 nit: Use \ to indicate the line breaks, so the command remains vaild and hence copy-paste-able to a shell. E.g. kudu cluster ksck --checksum_scan --checksum_snapshot=false \ --ksck_format=json_compact localhost:7052,localhost:7051,localhost:7053 http://gerrit.cloudera.org:8080/#/c/10535/6/src/kudu/tools/ksck-test.cc File src/kudu/tools/ksck-test.cc: http://gerrit.cloudera.org:8080/#/c/10535/6/src/kudu/tools/ksck-test.cc@56 PS6, Line 56: DECLARE_string(ksck_format); nit: Order alphabetically by flag name. http://gerrit.cloudera.org:8080/#/c/10535/6/src/kudu/tools/ksck-test.cc@1503 PS6, Line 1503: ASSERT_OK(r.Init()); Can you double-check this failed without this patch? I think JsonReader parses the whole json on Init() but want to be sure. http://gerrit.cloudera.org:8080/#/c/10535/6/src/kudu/tools/ksck.h File src/kudu/tools/ksck.h: http://gerrit.cloudera.org:8080/#/c/10535/6/src/kudu/tools/ksck.h@567 PS6, Line 567: (non-JSON) nit: Remove. Also, JSON is machine-readable. http://gerrit.cloudera.org:8080/#/c/10535/6/src/kudu/tools/ksck.h@567 PS6, Line 567: Check if nit: Return whether http://gerrit.cloudera.org:8080/#/c/10535/6/src/kudu/tools/ksck.h@567 PS6, Line 567: are is http://gerrit.cloudera.org:8080/#/c/10535/6/src/kudu/tools/ksck.h@568 PS6, Line 568: static bool IsMachineReadableFormat Can you make this a helper function in an anonymous namespace in ksck.cc rather that a static member function of Ksck? http://gerrit.cloudera.org:8080/#/c/10535/6/src/kudu/tools/ksck.cc File src/kudu/tools/ksck.cc: http://gerrit.cloudera.org:8080/#/c/10535/6/src/kudu/tools/ksck.cc@1063 PS6, Line 1063: FLAGS_ksck_format == "plain_full" || FLAGS_ksck_format == "plain_concise" We are usually case-insensitive about categorical flag values like this. Can you use boost::iequals to do the comparisons instead? -- To view, visit http://gerrit.cloudera.org:8080/10535 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I25fe6d0f14d1713b848d0a79f4d92b056924a5a5 Gerrit-Change-Number: 10535 Gerrit-PatchSet: 6 Gerrit-Owner: Fengling Wang <[email protected]> Gerrit-Reviewer: Fengling Wang <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Will Berkeley <[email protected]> Gerrit-Comment-Date: Mon, 11 Jun 2018 22:24:11 +0000 Gerrit-HasComments: Yes
