You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Mike Percy (Code Review)" <ge...@cloudera.org> on 2018/01/13 03:15:07 UTC
[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 <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
[kudu-CR] tools: Add debug mode to pb dump tool
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
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 <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 17 Jan 2018 06:22:28 +0000
Gerrit-HasComments: Yes
[kudu-CR] tools: Add debug mode to pb dump tool
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
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 <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] tools: Add debug mode to pb dump tool
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
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 <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Tue, 26 Jun 2018 18:53:38 +0000
Gerrit-HasComments: No
[kudu-CR] tools: Add debug mode to pb dump tool
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
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 <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] tools: Add debug mode to pb dump tool
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
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 <wd...@gmail.com>
---
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 <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] tools: Add debug mode to pb dump tool
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
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 <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 26 Jun 2018 03:10:44 +0000
Gerrit-HasComments: Yes
[kudu-CR] tools: Add debug mode to pb dump tool
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
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 <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Tue, 16 Jan 2018 18:41:35 +0000
Gerrit-HasComments: Yes