Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/9024 )
Change subject: tools: Add debug mode to pb dump tool ...................................................................... Patch Set 1: (5 comments) http://gerrit.cloudera.org:8080/#/c/9024/1//COMMIT_MSG Commit Message: PS1: Could you add a short test to kudu-tool-test? A run with --debug and a check for the presence of additional output should suffice. http://gerrit.cloudera.org:8080/#/c/9024/1//COMMIT_MSG@13 PS1, Line 13: TODO: Should this be called "verbose" mode instead of debug mode? I don't think it really matters, since the additional information is just for debugging. http://gerrit.cloudera.org:8080/#/c/9024/1/src/kudu/tools/tool_action_pbc.cc File src/kudu/tools/tool_action_pbc.cc: http://gerrit.cloudera.org:8080/#/c/9024/1/src/kudu/tools/tool_action_pbc.cc@92 PS1, Line 92: } Nit: could you add an else that does LOG(FATAL) or something like that? http://gerrit.cloudera.org:8080/#/c/9024/1/src/kudu/tools/tool_action_pbc.cc@232 PS1, Line 232: .AddOptionalParameter("oneline") Should add debug here. http://gerrit.cloudera.org:8080/#/c/9024/1/src/kudu/util/pb_util.cc File src/kudu/util/pb_util.cc: http://gerrit.cloudera.org:8080/#/c/9024/1/src/kudu/util/pb_util.cc@937 PS1, Line 937: "-------" Nit: if this is now being used in three places, perhaps make it a constant so there's no worry about the number of dashes being different in each of the three places? -- To view, visit http://gerrit.cloudera.org:8080/9024 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ica2a74a2e252d140cf7704ff68037a64db19cd80 Gerrit-Change-Number: 9024 Gerrit-PatchSet: 1 Gerrit-Owner: Mike Percy <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Comment-Date: Tue, 16 Jan 2018 18:41:35 +0000 Gerrit-HasComments: Yes
