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 11: (10 comments) Note also that the plain_* output is out of date. Based on a comment from Alexey I made sure there's a newline between different tablet summaries. 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? Done 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 Done http://gerrit.cloudera.org:8080/#/c/10288/11/src/kudu/tools/ksck-test.cc@377 PS11, Line 377: json > nit: JSON data Done http://gerrit.cloudera.org:8080/#/c/10288/11/src/kudu/tools/ksck-test.cc@424 PS11, Line 424: const KsckServerHealthSummary > const ExtractInt64& Done 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 Done 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! Done http://gerrit.cloudera.org:8080/#/c/10288/11/src/kudu/tools/ksck_results.h@284 PS11, Line 284: json > nit: JSON Done 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' di Done 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()? Fr Yeah, I think we generally don't check for this anywhere, and if I check it here I ought to write a big patch to check everywhere. 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 enumerat Will follow existing convention. -- 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 19:12:39 +0000 Gerrit-HasComments: Yes
