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 11: (10 comments) Overall LGTM, just a few nits. http://gerrit.cloudera.org:8080/#/c/10288/11/src/kudu/tools/ksck-test.cc File src/kudu/tools/ksck-test.cc: http://gerrit.cloudera.org:8080/#/c/10288/11/src/kudu/tools/ksck-test.cc@20 PS11, Line 20: #include <stdint.h> nit: could you update this to be C++-style include? #include <cstdint> Yes, IWYU recommendations are a bit inconsistent in that regard, hopefully it will be fixed someday. http://gerrit.cloudera.org:8080/#/c/10288/11/src/kudu/tools/ksck-test.cc@358 PS11, Line 358: stringstream Consider using ostringstream since here input-related functionality is not needed. http://gerrit.cloudera.org:8080/#/c/10288/11/src/kudu/tools/ksck-test.cc@377 PS11, Line 377: json nit: JSON data http://gerrit.cloudera.org:8080/#/c/10288/11/src/kudu/tools/ksck-test.cc@424 PS11, Line 424: const KsckServerHealthSummary const ExtractInt64& or maybe just const auto& ? http://gerrit.cloudera.org:8080/#/c/10288/11/src/kudu/tools/ksck_results.h File src/kudu/tools/ksck_results.h: http://gerrit.cloudera.org:8080/#/c/10288/11/src/kudu/tools/ksck_results.h@19 PS11, Line 19: #include <stdint.h> nit: replace with C++-style include #include <cstdint> http://gerrit.cloudera.org:8080/#/c/10288/11/src/kudu/tools/ksck_results.h@245 PS11, Line 245: Differs from JSON_PRETTY only in : // format, not content. Thank you for adding this! http://gerrit.cloudera.org:8080/#/c/10288/11/src/kudu/tools/ksck_results.h@284 PS11, Line 284: json nit: JSON http://gerrit.cloudera.org:8080/#/c/10288/11/src/kudu/tools/ksck_results.cc File src/kudu/tools/ksck_results.cc: http://gerrit.cloudera.org:8080/#/c/10288/11/src/kudu/tools/ksck_results.cc@628 PS11, Line 628: std:: nit: the namespace could be dropped because of the corresponding 'using' directive. http://gerrit.cloudera.org:8080/#/c/10288/11/src/kudu/tools/ksck_results.cc@638 PS11, Line 638: return Status::OK(); nit: maybe, check for stream error state before returning Status::OK()? From the other side, I didn't see such a check done anywhere in the Kudu sources. Maybe, it's OK as is because such type of errors a highly unlikely to happen. http://gerrit.cloudera.org:8080/#/c/10288/11/src/kudu/tools/tool.proto File src/kudu/tools/tool.proto: http://gerrit.cloudera.org:8080/#/c/10288/11/src/kudu/tools/tool.proto@219 PS11, Line 219: UNKNOWN = 999; I remember Dan advocated for using 0 for the default values of the enumerations which is to be in-line with protobuf3 conventions, but if you want it to be in line with the convention used in Kudu protobuf files so far, that's OK, I think. -- 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: 11 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: Fri, 11 May 2018 18:50:21 +0000 Gerrit-HasComments: Yes
