[kudu-CR] tool: port log-dump
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 DemboGerrit-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 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 DemboGerrit-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 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 DemboGerrit-Reviewer: Dinesh Bhat Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] tool: port log-dump
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 DemboGerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] tool: port log-dump
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
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 DemboGerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] tool: port log-dump
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 DemboGerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] tool: port log-dump
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 DemboGerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] tool: port log-dump
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 DemboGerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No