[kudu-CR] tools: Add debug mode to pb dump tool
Mike Percy has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/9024 ) Change subject: tools: Add debug mode to pb dump tool .. tools: Add debug mode to pb dump tool This is useful when dumping a partially-flushed file to determine the start of the damaged entry. To remove the damaged entry, the file can be truncated at the beginning of the offending entry. See KUDU-2260. Change-Id: Ica2a74a2e252d140cf7704ff68037a64db19cd80 Reviewed-on: http://gerrit.cloudera.org:8080/9024 Tested-by: Kudu Jenkins Reviewed-by: Will Berkeley --- M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool_action_pbc.cc M src/kudu/util/pb_util.cc M src/kudu/util/pb_util.h 4 files changed, 59 insertions(+), 3 deletions(-) Approvals: Kudu Jenkins: Verified Will Berkeley: Looks good to me, approved -- 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: merged Gerrit-Change-Id: Ica2a74a2e252d140cf7704ff68037a64db19cd80 Gerrit-Change-Number: 9024 Gerrit-PatchSet: 4 Gerrit-Owner: Mike Percy Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley
[kudu-CR] tools: Add debug mode to pb dump tool
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/9024 ) Change subject: tools: Add debug mode to pb dump tool .. Patch Set 3: Code-Review+2 -- 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: 3 Gerrit-Owner: Mike Percy Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Tue, 26 Jun 2018 18:53:38 + Gerrit-HasComments: No
[kudu-CR] tools: Add debug mode to pb dump tool
Hello Will Berkeley, Tidy Bot, Kudu Jenkins, Adar Dembo, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/9024 to look at the new patch set (#3). Change subject: tools: Add debug mode to pb dump tool .. tools: Add debug mode to pb dump tool This is useful when dumping a partially-flushed file to determine the start of the damaged entry. To remove the damaged entry, the file can be truncated at the beginning of the offending entry. See KUDU-2260. Change-Id: Ica2a74a2e252d140cf7704ff68037a64db19cd80 --- M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool_action_pbc.cc M src/kudu/util/pb_util.cc M src/kudu/util/pb_util.h 4 files changed, 59 insertions(+), 3 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/24/9024/3 -- 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: newpatchset Gerrit-Change-Id: Ica2a74a2e252d140cf7704ff68037a64db19cd80 Gerrit-Change-Number: 9024 Gerrit-PatchSet: 3 Gerrit-Owner: Mike Percy Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley
[kudu-CR] tools: Add debug mode to pb dump tool
Hello Kudu Jenkins, Adar Dembo, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/9024 to look at the new patch set (#2). Change subject: tools: Add debug mode to pb dump tool .. tools: Add debug mode to pb dump tool This is useful when dumping a partially-flushed file to determine the start of the damaged entry. To remove the damaged entry, the file can be truncated at the beginning of the offending entry. See KUDU-2260. Change-Id: Ica2a74a2e252d140cf7704ff68037a64db19cd80 --- M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool_action_pbc.cc M src/kudu/util/pb_util.cc M src/kudu/util/pb_util.h 4 files changed, 58 insertions(+), 2 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/24/9024/2 -- 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: newpatchset Gerrit-Change-Id: Ica2a74a2e252d140cf7704ff68037a64db19cd80 Gerrit-Change-Number: 9024 Gerrit-PatchSet: 2 Gerrit-Owner: Mike Percy Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon
[kudu-CR] tools: Add debug mode to pb dump tool
Mike Percy 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:
(6 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 chec
Done
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 f
Done
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?
That's not necessary since the flags are all optional.
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.
Done
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@930
PS1, Line 930: // Fallthrough.
> use FALLTHROUGH_INTENDED from macros.h which has some magic effect on clang
removed the fallthrough
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
Done
--
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
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy
Gerrit-Reviewer: Todd Lipcon
Gerrit-Comment-Date: Tue, 26 Jun 2018 03:10:44 +
Gerrit-HasComments: Yes
[kudu-CR] tools: Add debug mode to pb dump tool
Todd Lipcon 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: (1 comment) 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@930 PS1, Line 930: // Fallthrough. use FALLTHROUGH_INTENDED from macros.h which has some magic effect on clang. Or, since this is just the common case of multiple case labels, and not a case where there is some code for DEFAULT which then falls through to DEBUG, I don't think it's really necessary in the first place. Mostly I like to document fallthrough in cases like: case A: some code; some more code; case B: even more code; break; because it looks like someone forgot the 'break' on case A. -- 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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 17 Jan 2018 06:22:28 + Gerrit-HasComments: Yes
[kudu-CR] tools: Add debug mode to pb dump tool
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
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Tue, 16 Jan 2018 18:41:35 +
Gerrit-HasComments: Yes
[kudu-CR] tools: Add debug mode to pb dump tool
Hello Adar Dembo, I'd like you to do a code review. Please visit http://gerrit.cloudera.org:8080/9024 to review the following change. Change subject: tools: Add debug mode to pb dump tool .. tools: Add debug mode to pb dump tool This is useful when dumping a partially-flushed file to determine the start of the damaged entry. To remove the damaged entry, the file can be truncated at the beginning of the offending entry. See KUDU-2260. TODO: Should this be called "verbose" mode instead of debug mode? Change-Id: Ica2a74a2e252d140cf7704ff68037a64db19cd80 --- M src/kudu/tools/tool_action_pbc.cc M src/kudu/util/pb_util.cc M src/kudu/util/pb_util.h 3 files changed, 36 insertions(+), 2 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/24/9024/1 -- 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: newchange Gerrit-Change-Id: Ica2a74a2e252d140cf7704ff68037a64db19cd80 Gerrit-Change-Number: 9024 Gerrit-PatchSet: 1 Gerrit-Owner: Mike Percy Gerrit-Reviewer: Adar Dembo
