[kudu-CR] tool: port log-dump

[Dinesh Bhat (Code Review)]

Dinesh Bhat has posted comments on this change.

Change subject: tool: port log-dump
..


Patch Set 3:

> (3 comments)

Thank you for the responses here.

-- 
To view, visit http://gerrit.cloudera.org:8080/4167
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I14f048169c0b211e3c72fcd255c76dd59cbb05c9
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] tool: port log-dump

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: tool: port log-dump
..


Patch Set 3:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/4167/3/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:

PS3, Line 92: out
> Some basic paranoia Qn unrelated to your change: The string object itself s
The string _data_ itself is heap-allocated; std::string is just a small 
container object.


Line 369:   const Schema kSchemaWithIds(SchemaBuilder(kSchema).Build());
> I am curious to know the purpose of SchemaBuilder here ? Can't we directly 
Apparently not. I copied this from log-test-base.h because when I tried to use 
kSchema as-is in Log::Open(), I got a log that could not be read later on. I 
guess you need to rebuild the schema with column IDs like this. I agree that 
it's unintuitive; I was just trying to "get it to work".


http://gerrit.cloudera.org:8080/#/c/4167/3/src/kudu/tools/tool_action_tablet.cc
File src/kudu/tools/tool_action_tablet.cc:

PS3, Line 265: AddOptionalParameter
> I have been wondering about this while testing today: isn't fs_wal_dir supp
fs_wal_dir and fs_data_dirs are "optional" in the sense that they're gflags and 
thus can be omitted from the command line and the tool still run. However, 
FsManager::Open will fail if their values aren't provided.

This is confusing because all other gflags are optional. For a while I thought 
maybe we should add required (positional) parameters for these two, then map 
their values to gflags values when parsing. But then the number of positional 
parameters is quite high, and that's annoying to deal with (i.e. it's easy to 
get them out of order on the command line).

So in a nutshell, I'm not really sure how we should handle them. We could make 
it explicit that some keyval args (i.e. --fs_wal_dir) are required, but that's 
not Unix-y.


-- 
To view, visit http://gerrit.cloudera.org:8080/4167
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I14f048169c0b211e3c72fcd255c76dd59cbb05c9
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] tool: port log-dump

[Dinesh Bhat (Code Review)]

Dinesh Bhat has posted comments on this change.

Change subject: tool: port log-dump
..


Patch Set 3:

(4 comments)

Hi Adar, sorry I missed the train here, but these are more of curious questions 
than review comments as such so pls feel free to answer in your spare time.

http://gerrit.cloudera.org:8080/#/c/4167/3/docs/release_notes.adoc
File docs/release_notes.adoc:

Line 53:   implemented as `kudu wal dump` and `kudu tablet dump_wals`.
aha, nice to see this.. will add it in my change too.


http://gerrit.cloudera.org:8080/#/c/4167/3/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:

PS3, Line 92: out
Some basic paranoia Qn unrelated to your change: The string object itself seems 
to be on stack, I haven't looked into '=' overloading part of 'string',  but I 
wonder if out happens to carry all std::cout emitted from subprocess here and 
whether we ever need to worry about ulimit -s ?


Line 369:   const Schema kSchemaWithIds(SchemaBuilder(kSchema).Build());
I am curious to know the purpose of SchemaBuilder here ? Can't we directly use 
kSchema while opening log ?


http://gerrit.cloudera.org:8080/#/c/4167/3/src/kudu/tools/tool_action_tablet.cc
File src/kudu/tools/tool_action_tablet.cc:

PS3, Line 265: AddOptionalParameter
I have been wondering about this while testing today: isn't fs_wal_dir supposed 
to be a required parameter for dump actions ?


-- 
To view, visit http://gerrit.cloudera.org:8080/4167
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I14f048169c0b211e3c72fcd255c76dd59cbb05c9
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] tool: port log-dump

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: tool: port log-dump
..


Patch Set 2: Code-Review+2

-- 
To view, visit http://gerrit.cloudera.org:8080/4167
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I14f048169c0b211e3c72fcd255c76dd59cbb05c9
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] tool: port log-dump

[Todd Lipcon (Code Review)]

Todd Lipcon has submitted this change and it was merged.

Change subject: tool: port log-dump
..


tool: port log-dump

This one was more complicated, because log-dump can run against a single
file or an entire tablet. So I put all the common functionality in one place
and referenced it from both modes.

I consolidated similar gflags where it made sense to do so, and I tweaked
the endline handling for help generation so that each parameter is separated
from the next with an empty line.

Semantic changes from log-dump:
- If called with print_entries=no, will also print the footer.
- The print_headers flag is now print_meta, to be more consistent with 'kudu
  cfile dump'.

Change-Id: I14f048169c0b211e3c72fcd255c76dd59cbb05c9
Reviewed-on: http://gerrit.cloudera.org:8080/4167
Tested-by: Kudu Jenkins
Reviewed-by: Todd Lipcon 
---
M docs/release_notes.adoc
M src/kudu/consensus/CMakeLists.txt
M src/kudu/tools/CMakeLists.txt
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action.cc
M src/kudu/tools/tool_action.h
R src/kudu/tools/tool_action_common.cc
A src/kudu/tools/tool_action_common.h
M src/kudu/tools/tool_action_fs.cc
M src/kudu/tools/tool_action_tablet.cc
A src/kudu/tools/tool_action_wal.cc
M src/kudu/tools/tool_main.cc
M src/kudu/util/test_macros.h
13 files changed, 440 insertions(+), 160 deletions(-)

Approvals:
  Todd Lipcon: Looks good to me, approved
  Kudu Jenkins: Verified



-- 
To view, visit http://gerrit.cloudera.org:8080/4167
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: I14f048169c0b211e3c72fcd255c76dd59cbb05c9
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] tool: port log-dump

[Adar Dembo (Code Review)]

Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/4167

to look at the new patch set (#2).

Change subject: tool: port log-dump
..

tool: port log-dump

This one was more complicated, because log-dump can run against a single
file or an entire tablet. So I put all the common functionality in one place
and referenced it from both modes.

I consolidated similar gflags where it made sense to do so, and I tweaked
the endline handling for help generation so that each parameter is separated
from the next with an empty line.

Semantic changes from log-dump:
- If called with print_entries=no, will also print the footer.
- The print_headers flag is now print_meta, to be more consistent with 'kudu
  cfile dump'.

Change-Id: I14f048169c0b211e3c72fcd255c76dd59cbb05c9
---
M docs/release_notes.adoc
M src/kudu/consensus/CMakeLists.txt
M src/kudu/tools/CMakeLists.txt
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action.cc
M src/kudu/tools/tool_action.h
R src/kudu/tools/tool_action_common.cc
A src/kudu/tools/tool_action_common.h
M src/kudu/tools/tool_action_fs.cc
M src/kudu/tools/tool_action_tablet.cc
A src/kudu/tools/tool_action_wal.cc
M src/kudu/tools/tool_main.cc
M src/kudu/util/test_macros.h
13 files changed, 440 insertions(+), 160 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/67/4167/2
-- 
To view, visit http://gerrit.cloudera.org:8080/4167
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I14f048169c0b211e3c72fcd255c76dd59cbb05c9
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] tool: port log-dump

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: tool: port log-dump
..


Patch Set 2:

Build Started http://104.196.14.100/job/kudu-gerrit/3151/

-- 
To view, visit http://gerrit.cloudera.org:8080/4167
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I14f048169c0b211e3c72fcd255c76dd59cbb05c9
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] tool: port log-dump

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: tool: port log-dump
..


Patch Set 1:

Code lgtm. Can you add this to the release note with a pointer to 'kudu wal 
dump' and 'kudu tablet dump_wals'?

-- 
To view, visit http://gerrit.cloudera.org:8080/4167
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I14f048169c0b211e3c72fcd255c76dd59cbb05c9
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] tool: port log-dump

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: tool: port log-dump
..


Patch Set 1:

Build Started http://104.196.14.100/job/kudu-gerrit/3144/

-- 
To view, visit http://gerrit.cloudera.org:8080/4167
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I14f048169c0b211e3c72fcd255c76dd59cbb05c9
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No