[kudu-CR] tool: port kudu-fs dump, remove kudu-fs list, fs tool
Todd Lipcon has submitted this change and it was merged. Change subject: tool: port kudu-fs_dump, remove kudu-fs_list, fs_tool .. tool: port kudu-fs_dump, remove kudu-fs_list, fs_tool This change ports fs_dump actions under 'kudu local_replica '. Additionally this has following re-organizations: - moved dump_cfile action under 'kudu fs dump cfile'. - kudu-fs_list tool has been removed altogether, but some of the functionalities are retained under 'local_replica' and 'fs dump' sub-actions. - fs_tool library is stripped off, and all those routines are in respective action files. Also added tests under kudu-tool-test to exercise each of these fs tools. Change-Id: I1ec628b65613011d8c48b6239c13762276425966 Reviewed-on: http://gerrit.cloudera.org:8080/4305 Tested-by: Kudu Jenkins Reviewed-by: Todd Lipcon --- M docs/release_notes.adoc M src/kudu/integration-tests/master_migration-itest.cc M src/kudu/tablet/rowset_metadata.h M src/kudu/tools/CMakeLists.txt D src/kudu/tools/fs_tool.cc D src/kudu/tools/fs_tool.h M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool_action.h M src/kudu/tools/tool_action_fs.cc M src/kudu/tools/tool_action_local_replica.cc M src/kudu/tools/tool_action_tablet.cc 11 files changed, 766 insertions(+), 823 deletions(-) Approvals: Todd Lipcon: Looks good to me, approved Kudu Jenkins: Verified -- To view, visit http://gerrit.cloudera.org:8080/4305 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: merged Gerrit-Change-Id: I1ec628b65613011d8c48b6239c13762276425966 Gerrit-PatchSet: 15 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dinesh Bhat Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dinesh Bhat Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] tool: port kudu-fs dump, remove kudu-fs list, fs tool
Todd Lipcon has posted comments on this change. Change subject: tool: port kudu-fs_dump, remove kudu-fs_list, fs_tool .. Patch Set 14: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/4305 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1ec628b65613011d8c48b6239c13762276425966 Gerrit-PatchSet: 14 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dinesh Bhat Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dinesh Bhat Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] tool: port kudu-fs dump, remove kudu-fs list, fs tool
Todd Lipcon has posted comments on this change. Change subject: tool: port kudu-fs_dump, remove kudu-fs_list, fs_tool .. Patch Set 8: Just rebased and put this in a series with Dinesh's patch. Will commit if it passes -- To view, visit http://gerrit.cloudera.org:8080/4305 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1ec628b65613011d8c48b6239c13762276425966 Gerrit-PatchSet: 8 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dinesh Bhat Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dinesh Bhat Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] tool: port kudu-fs dump, remove kudu-fs list, fs tool
Hello Adar Dembo, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/4305 to look at the new patch set (#14). Change subject: tool: port kudu-fs_dump, remove kudu-fs_list, fs_tool .. tool: port kudu-fs_dump, remove kudu-fs_list, fs_tool This change ports fs_dump actions under 'kudu local_replica '. Additionally this has following re-organizations: - moved dump_cfile action under 'kudu fs dump cfile'. - kudu-fs_list tool has been removed altogether, but some of the functionalities are retained under 'local_replica' and 'fs dump' sub-actions. - fs_tool library is stripped off, and all those routines are in respective action files. Also added tests under kudu-tool-test to exercise each of these fs tools. Change-Id: I1ec628b65613011d8c48b6239c13762276425966 --- M docs/release_notes.adoc M src/kudu/integration-tests/master_migration-itest.cc M src/kudu/tablet/rowset_metadata.h M src/kudu/tools/CMakeLists.txt D src/kudu/tools/fs_tool.cc D src/kudu/tools/fs_tool.h M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool_action.h M src/kudu/tools/tool_action_fs.cc M src/kudu/tools/tool_action_local_replica.cc M src/kudu/tools/tool_action_tablet.cc 11 files changed, 766 insertions(+), 823 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/05/4305/14 -- To view, visit http://gerrit.cloudera.org:8080/4305 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I1ec628b65613011d8c48b6239c13762276425966 Gerrit-PatchSet: 14 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dinesh Bhat Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dinesh Bhat Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] tool: port kudu-fs dump, remove kudu-fs list, fs tool
Kudu Jenkins has posted comments on this change. Change subject: tool: port kudu-fs_dump, remove kudu-fs_list, fs_tool .. Patch Set 14: Build Started http://104.196.14.100/job/kudu-gerrit/3391/ -- To view, visit http://gerrit.cloudera.org:8080/4305 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1ec628b65613011d8c48b6239c13762276425966 Gerrit-PatchSet: 14 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dinesh Bhat Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dinesh Bhat Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] tool: port kudu-fs dump, remove kudu-fs list, fs tool
Adar Dembo has posted comments on this change. Change subject: tool: port kudu-fs_dump, remove kudu-fs_list, fs_tool .. Patch Set 13: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/4305 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1ec628b65613011d8c48b6239c13762276425966 Gerrit-PatchSet: 13 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dinesh Bhat Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dinesh Bhat Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] tool: port kudu-fs dump, remove kudu-fs list, fs tool
Dinesh Bhat has posted comments on this change. Change subject: tool: port kudu-fs_dump, remove kudu-fs_list, fs_tool .. Patch Set 13: (2 comments) http://gerrit.cloudera.org:8080/#/c/4305/9/src/kudu/tools/tool_action_local_replica.cc File src/kudu/tools/tool_action_local_replica.cc: PS9, Line 679: > Well, I think "tablet_id" does make more sense than something like "replica cool, I adjusted few anomalies I found about 80 chars at few places. It wasn't related to this thread/comment, just some observation around these lines I think. http://gerrit.cloudera.org:8080/#/c/4305/12/src/kudu/tools/tool_action_local_replica.cc File src/kudu/tools/tool_action_local_replica.cc: PS12, Line 676: _config", &Rewrite > Nit: should be "local Kudu replica" to be consistent with the description o Done -- To view, visit http://gerrit.cloudera.org:8080/4305 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1ec628b65613011d8c48b6239c13762276425966 Gerrit-PatchSet: 13 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dinesh Bhat Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dinesh Bhat Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] tool: port kudu-fs dump, remove kudu-fs list, fs tool
Kudu Jenkins has posted comments on this change. Change subject: tool: port kudu-fs_dump, remove kudu-fs_list, fs_tool .. Patch Set 13: Build Started http://104.196.14.100/job/kudu-gerrit/3378/ -- To view, visit http://gerrit.cloudera.org:8080/4305 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1ec628b65613011d8c48b6239c13762276425966 Gerrit-PatchSet: 13 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dinesh Bhat Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dinesh Bhat Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] tool: port kudu-fs dump, remove kudu-fs list, fs tool
Hello Adar Dembo, Todd Lipcon, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/4305 to look at the new patch set (#13). Change subject: tool: port kudu-fs_dump, remove kudu-fs_list, fs_tool .. tool: port kudu-fs_dump, remove kudu-fs_list, fs_tool This change ports fs_dump actions under 'kudu local_replica '. Additionally this has following re-organizations: - moved dump_cfile action under 'kudu fs dump cfile'. - kudu-fs_list tool has been removed altogether, but some of the functionalities are retained under 'local_replica' and 'fs dump' sub-actions. - fs_tool library is stripped off, and all those routines are in respective action files. Also added tests under kudu-tool-test to exercise each of these fs tools. Change-Id: I1ec628b65613011d8c48b6239c13762276425966 --- M docs/release_notes.adoc M src/kudu/integration-tests/master_migration-itest.cc M src/kudu/tablet/rowset_metadata.h M src/kudu/tools/CMakeLists.txt D src/kudu/tools/fs_tool.cc D src/kudu/tools/fs_tool.h M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool_action.h M src/kudu/tools/tool_action_fs.cc M src/kudu/tools/tool_action_local_replica.cc M src/kudu/tools/tool_action_tablet.cc 11 files changed, 766 insertions(+), 823 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/05/4305/13 -- To view, visit http://gerrit.cloudera.org:8080/4305 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I1ec628b65613011d8c48b6239c13762276425966 Gerrit-PatchSet: 13 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dinesh Bhat Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dinesh Bhat Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] tool: port kudu-fs dump, remove kudu-fs list, fs tool
Adar Dembo has posted comments on this change. Change subject: tool: port kudu-fs_dump, remove kudu-fs_list, fs_tool .. Patch Set 9: (2 comments) http://gerrit.cloudera.org:8080/#/c/4305/9/src/kudu/tools/tool_action_local_replica.cc File src/kudu/tools/tool_action_local_replica.cc: PS9, Line 679: tablet > yikes !! sorry, actually it's kinda odd that all our required params are 't Well, I think "tablet_id" does make more sense than something like "replica_id", because the identifier is unique to the tablet rather than to the replica. That is, all replicas of this tablet share this same identifier, so it is not a useful way of identifying this replica. And yes, we should still wrap at 80 chars; I'm not sure how this thread suggests otherwise? http://gerrit.cloudera.org:8080/#/c/4305/12/src/kudu/tools/tool_action_local_replica.cc File src/kudu/tools/tool_action_local_replica.cc: PS12, Line 676: Nit: should be "local Kudu replica" to be consistent with the description of the local_replica mode. -- To view, visit http://gerrit.cloudera.org:8080/4305 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1ec628b65613011d8c48b6239c13762276425966 Gerrit-PatchSet: 9 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dinesh Bhat Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dinesh Bhat Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] tool: port kudu-fs dump, remove kudu-fs list, fs tool
Dinesh Bhat has posted comments on this change. Change subject: tool: port kudu-fs_dump, remove kudu-fs_list, fs_tool .. Patch Set 11: (6 comments) http://gerrit.cloudera.org:8080/#/c/4305/9/src/kudu/tools/tool_action_local_replica.cc File src/kudu/tools/tool_action_local_replica.cc: Line 70: DEFINE_int64(rowset_index, INT64_MAX, > Why is INT64_MAX a better choice than -1? The advantage of -1 is that it's Absence of this flags on cmd-line will mean -1, which means even if we type '--rowset_index=-1' by mistake we end up printing everything. I preferred INT64_MAX over that. Given that this is meant for adv users, INT64_MAX kinda felt intuitive. I changed it back to (-1) anyways because I wasn't against either approach. Line 384:nullptr, // MetricRegistry > The MRS will be empty if the tablet isn't bootstrapped. So in effect, it's I see, ok removed now. removed fomr test too. PS9, Line 679: > Nope, not changed. yikes !! sorry, actually it's kinda odd that all our required params are 'tablet_id' and help strings are not referring to tablet anymore :). But, laters may be. Also do we not want to honor 80 char wrap-ups ? PS9, Line 721: > Looks like you missed this one. Done http://gerrit.cloudera.org:8080/#/c/4305/11/src/kudu/tools/tool_action_local_replica.cc File src/kudu/tools/tool_action_local_replica.cc: PS11, Line 637: tablet > replica Done PS11, Line 645: tablet > replica Done -- To view, visit http://gerrit.cloudera.org:8080/4305 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1ec628b65613011d8c48b6239c13762276425966 Gerrit-PatchSet: 11 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dinesh Bhat Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dinesh Bhat Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] tool: port kudu-fs dump, remove kudu-fs list, fs tool
Hello Adar Dembo, Todd Lipcon, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/4305 to look at the new patch set (#12). Change subject: tool: port kudu-fs_dump, remove kudu-fs_list, fs_tool .. tool: port kudu-fs_dump, remove kudu-fs_list, fs_tool This change ports fs_dump actions under 'kudu local_replica '. Additionally this has following re-organizations: - moved dump_cfile action under 'kudu fs dump cfile'. - kudu-fs_list tool has been removed altogether, but some of the functionalities are retained under 'local_replica' and 'fs dump' sub-actions. - fs_tool library is stripped off, and all those routines are in respective action files. Also added tests under kudu-tool-test to exercise each of these fs tools. Change-Id: I1ec628b65613011d8c48b6239c13762276425966 --- M docs/release_notes.adoc M src/kudu/integration-tests/master_migration-itest.cc M src/kudu/tablet/rowset_metadata.h M src/kudu/tools/CMakeLists.txt D src/kudu/tools/fs_tool.cc D src/kudu/tools/fs_tool.h M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool_action.h M src/kudu/tools/tool_action_fs.cc M src/kudu/tools/tool_action_local_replica.cc M src/kudu/tools/tool_action_tablet.cc 11 files changed, 748 insertions(+), 819 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/05/4305/12 -- To view, visit http://gerrit.cloudera.org:8080/4305 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I1ec628b65613011d8c48b6239c13762276425966 Gerrit-PatchSet: 12 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dinesh Bhat Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dinesh Bhat Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] tool: port kudu-fs dump, remove kudu-fs list, fs tool
Kudu Jenkins has posted comments on this change. Change subject: tool: port kudu-fs_dump, remove kudu-fs_list, fs_tool .. Patch Set 12: Build Started http://104.196.14.100/job/kudu-gerrit/3374/ -- To view, visit http://gerrit.cloudera.org:8080/4305 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1ec628b65613011d8c48b6239c13762276425966 Gerrit-PatchSet: 12 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dinesh Bhat Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dinesh Bhat Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] tool: port kudu-fs dump, remove kudu-fs list, fs tool
Adar Dembo has posted comments on this change. Change subject: tool: port kudu-fs_dump, remove kudu-fs_list, fs_tool .. Patch Set 11: (8 comments) http://gerrit.cloudera.org:8080/#/c/4305/9/src/kudu/tools/tool_action_local_replica.cc File src/kudu/tools/tool_action_local_replica.cc: Line 70: DEFINE_int64(rowset_index, INT64_MAX, > Err... updated to INT64_MAX. Why is INT64_MAX a better choice than -1? The advantage of -1 is that it's actually printed in the help as "-1", whereas INT64_MAX looks like some huge number that a user might think has meaning as a real number. Line 384:nullptr, // MetricRegistry > I can take it out, but it looked like it was dumping some in-memory content The MRS will be empty if the tablet isn't bootstrapped. So in effect, it's still doing the same thing as "dump rowset". Plus it's doing it in a poor way: it loads _all_ the data into an in-memory vector, and the data is loaded as debug strings, which aren't as useful as how "dump rowset" prints. PS9, Line 521: } : > Done, doesn't the 4-space indent next-line apply here ? Generally yes, but sometimes people deviate from that style to "pretty print" some stuff (like this), and when they do, I think it's better code etiquette to preserve that style, at least on a file-by-file basis. PS9, Line 607: // Rowset index no > Heh, somewhere came across in Strostrup book few weeks ago, this way of ini Primitive types don't have constructors/destructors the way that classes and structs do. It's all just raw memory. PS9, Line 679: > Done Nope, not changed. PS9, Line 721: > Should be 'replica' now. Looks like you missed this one. http://gerrit.cloudera.org:8080/#/c/4305/11/src/kudu/tools/tool_action_local_replica.cc File src/kudu/tools/tool_action_local_replica.cc: PS11, Line 637: tablet replica PS11, Line 645: tablet replica -- To view, visit http://gerrit.cloudera.org:8080/4305 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1ec628b65613011d8c48b6239c13762276425966 Gerrit-PatchSet: 11 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dinesh Bhat Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dinesh Bhat Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] tool: port kudu-fs dump, remove kudu-fs list, fs tool
Dinesh Bhat has posted comments on this change. Change subject: tool: port kudu-fs_dump, remove kudu-fs_list, fs_tool .. Patch Set 11: (1 comment) http://gerrit.cloudera.org:8080/#/c/4305/9/src/kudu/tools/tool_action_local_replica.cc File src/kudu/tools/tool_action_local_replica.cc: Line 70: DEFINE_int64(rowset_index, INT64_MAX, > Yeah, plus checking with if(FLAGS_rowset_index) would yield unexpected resu Err... updated to INT64_MAX. -- To view, visit http://gerrit.cloudera.org:8080/4305 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1ec628b65613011d8c48b6239c13762276425966 Gerrit-PatchSet: 11 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dinesh Bhat Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dinesh Bhat Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] tool: port kudu-fs dump, remove kudu-fs list, fs tool
Kudu Jenkins has posted comments on this change. Change subject: tool: port kudu-fs_dump, remove kudu-fs_list, fs_tool .. Patch Set 11: Build Started http://104.196.14.100/job/kudu-gerrit/3370/ -- To view, visit http://gerrit.cloudera.org:8080/4305 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1ec628b65613011d8c48b6239c13762276425966 Gerrit-PatchSet: 11 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dinesh Bhat Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dinesh Bhat Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] tool: port kudu-fs dump, remove kudu-fs list, fs tool
Hello Adar Dembo, Todd Lipcon, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/4305 to look at the new patch set (#11). Change subject: tool: port kudu-fs_dump, remove kudu-fs_list, fs_tool .. tool: port kudu-fs_dump, remove kudu-fs_list, fs_tool This change ports fs_dump actions under 'kudu local_replica '. Additionally this has following re-organizations: - moved dump_cfile action under 'kudu fs dump cfile'. - kudu-fs_list tool has been removed altogether, but some of the functionalities are retained under 'local_replica' and 'fs dump' sub-actions. - fs_tool library is stripped off, and all those routines are in respective action files. Also added tests under kudu-tool-test to exercise each of these fs tools. Change-Id: I1ec628b65613011d8c48b6239c13762276425966 --- M docs/release_notes.adoc M src/kudu/integration-tests/master_migration-itest.cc M src/kudu/tablet/rowset_metadata.h M src/kudu/tools/CMakeLists.txt D src/kudu/tools/fs_tool.cc D src/kudu/tools/fs_tool.h M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool_action.h M src/kudu/tools/tool_action_fs.cc M src/kudu/tools/tool_action_local_replica.cc M src/kudu/tools/tool_action_tablet.cc 11 files changed, 792 insertions(+), 817 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/05/4305/11 -- To view, visit http://gerrit.cloudera.org:8080/4305 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I1ec628b65613011d8c48b6239c13762276425966 Gerrit-PatchSet: 11 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dinesh Bhat Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dinesh Bhat Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] tool: port kudu-fs dump, remove kudu-fs list, fs tool
Kudu Jenkins has posted comments on this change. Change subject: tool: port kudu-fs_dump, remove kudu-fs_list, fs_tool .. Patch Set 10: Build Started http://104.196.14.100/job/kudu-gerrit/3368/ -- To view, visit http://gerrit.cloudera.org:8080/4305 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1ec628b65613011d8c48b6239c13762276425966 Gerrit-PatchSet: 10 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dinesh Bhat Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dinesh Bhat Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] tool: port kudu-fs dump, remove kudu-fs list, fs tool
Hello Adar Dembo, Todd Lipcon, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/4305 to look at the new patch set (#10). Change subject: tool: port kudu-fs_dump, remove kudu-fs_list, fs_tool .. tool: port kudu-fs_dump, remove kudu-fs_list, fs_tool This change ports fs_dump actions under 'kudu local_replica '. Additionally this has following re-organizations: - moved dump_cfile action under 'kudu fs dump cfile'. - kudu-fs_list tool has been removed altogether, but some of the functionalities are retained under 'local_replica' and 'fs dump' sub-actions. - fs_tool library is stripped off, and all those routines are in respective action files. Also added tests under kudu-tool-test to exercise each of these fs tools. Change-Id: I1ec628b65613011d8c48b6239c13762276425966 --- M docs/release_notes.adoc M src/kudu/integration-tests/master_migration-itest.cc M src/kudu/tablet/rowset_metadata.h M src/kudu/tools/CMakeLists.txt D src/kudu/tools/fs_tool.cc D src/kudu/tools/fs_tool.h M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool_action.h M src/kudu/tools/tool_action_fs.cc M src/kudu/tools/tool_action_local_replica.cc M src/kudu/tools/tool_action_tablet.cc 11 files changed, 792 insertions(+), 817 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/05/4305/10 -- To view, visit http://gerrit.cloudera.org:8080/4305 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I1ec628b65613011d8c48b6239c13762276425966 Gerrit-PatchSet: 10 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dinesh Bhat Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dinesh Bhat Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] tool: port kudu-fs dump, remove kudu-fs list, fs tool
Dinesh Bhat has posted comments on this change.
Change subject: tool: port kudu-fs_dump, remove kudu-fs_list, fs_tool
..
Patch Set 9:
(12 comments)
http://gerrit.cloudera.org:8080/#/c/4305/8/src/kudu/tools/tool_action_local_replica.cc
File src/kudu/tools/tool_action_local_replica.cc:
PS8, Line 114:
> In the latest diff (PS9) it's still there.
Done
PS8, Line 476:
: // TODO: it's awkward that whenever we want to iterate over
deltas we also
: // need to open the CFileSet for the rowset. Ide
> Yes, but you didn't update the comment to mention unique_ptr instead of gsc
Done
PS8, Line 682: .AddOptionalParameter("fs_data_dirs")
> But the two aren't equivalent. "All rowsets" means all on-disk rowsets, whi
What I meant was 'dump blocks' was dumping all rowsets for a tablet_id, only
diff between the two actions were row_index, hence combined them now.
http://gerrit.cloudera.org:8080/#/c/4305/9/src/kudu/tools/tool_action_local_replica.cc
File src/kudu/tools/tool_action_local_replica.cc:
Line 70: DEFINE_int64(rowset_index, 0, "Index of the rowset in local replica");
> But rowset index 0 is a valid index. You should default to -1 here, and mod
Yeah, plus checking with if(FLAGS_rowset_index) would yield unexpected results
as well. I am using INT_MAX as default value instead of -1. Updated help
accordingly.
PS9, Line 139: fs_ptr->reset(new FsManager(Env::Default(), fs_opts));
: RETURN_NOT_OK((*fs_ptr)->Open());
> Nit: generally, we prefer to use the calling convention where the OUT param
Done
Line 384: Status DumpData(const RunnerContext& context) {
> I took a look at how this is implemented, and I think we should remove it t
I can take it out, but it looked like it was dumping some in-memory contents
too(MRS), and wasn't sure about removing it. Perhaps we can consolidate 'dump
data' and 'dump rowset' post 1.0 ? I didn't want to remove altogether and
re-add later if we find something missing.
PS9, Line 434: bool metadata_only
> Every time this function is called, FLAGS_metadata_only is fed into this ar
Done
PS9, Line 521: cout << Indent(indent) << upd.key.ToString() << " "
: << RowChangeList(upd.cell).ToString(schema) << endl;
> The old code (before removing the std:: prefixes) took care to align the <<
Done, doesn't the 4-space indent next-line apply here ?
PS9, Line 607: bool found{false};
> What's this? Why not "bool found = false;"?
Heh, somewhere came across in Strostrup book few weeks ago, this way of
initializing is supported for built-in types too, but I am curious if the code
in 'found = false' would resort to invoking constructor too for built-in types.
May be not.
PS9, Line 667: tablet
> Replica.
Done
PS9, Line 669: .AddOptionalParameter("rowset_index")
> Resort this list of parameters alphabetically.
Done
PS9, Line 679: tablet
> Should now be 'replica'.
Done
--
To view, visit http://gerrit.cloudera.org:8080/4305
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I1ec628b65613011d8c48b6239c13762276425966
Gerrit-PatchSet: 9
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Dinesh Bhat
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon
Gerrit-HasComments: Yes
[kudu-CR] tool: port kudu-fs dump, remove kudu-fs list, fs tool
Adar Dembo has posted comments on this change.
Change subject: tool: port kudu-fs_dump, remove kudu-fs_list, fs_tool
..
Patch Set 9:
(14 comments)
http://gerrit.cloudera.org:8080/#/c/4305/9/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:
PS9, Line 687: kRownId
kRowId
http://gerrit.cloudera.org:8080/#/c/4305/8/src/kudu/tools/tool_action_local_replica.cc
File src/kudu/tools/tool_action_local_replica.cc:
PS8, Line 114:
> Agreed, done.
In the latest diff (PS9) it's still there.
PS8, Line 476:
: // TODO: it's awkward that whenever we want to iterate over
deltas we also
: // need to open the CFileSet for the rowset. Ide
> Done
Yes, but you didn't update the comment to mention unique_ptr instead of
gscoped_ptr.
PS8, Line 682: .AddOptionalParameter("fs_data_dirs")
> Very interesting catch ! it turns out dump blocks was nothing but dumping a
But the two aren't equivalent. "All rowsets" means all on-disk rowsets, which
means all on-disk data blocks.
http://gerrit.cloudera.org:8080/#/c/4305/9/src/kudu/tools/tool_action_local_replica.cc
File src/kudu/tools/tool_action_local_replica.cc:
Line 70: DEFINE_int64(rowset_index, 0, "Index of the rowset in local replica");
But rowset index 0 is a valid index. You should default to -1 here, and modify
the help text to explain the special value.
PS9, Line 139: fs_ptr->reset(new FsManager(Env::Default(), fs_opts));
: RETURN_NOT_OK((*fs_ptr)->Open());
Nit: generally, we prefer to use the calling convention where the OUT parameter
(fs_ptr in this case) isn't modified unless the function succeeds. So what you
should do is store the new FsManager in a local unique_ptr, then after Open()
succeeds, swap the local unique_ptr with fs_ptr.
Line 384: Status DumpData(const RunnerContext& context) {
I took a look at how this is implemented, and I think we should remove it too.
The problem is that it purports to dumping all of the data of the tablet, but
it doesn't actually bootstrap the tablet, so any data in WAL segments is
missed. After that, it's functionally equivalent to DumpRowSet (with
rowset_index==0) in that it'll just dump all the on-disk blocks.
PS9, Line 434: bool metadata_only
Every time this function is called, FLAGS_metadata_only is fed into this
argument, so just drop the argument and access FLAGS_metadata_only directly
inside the function.
PS9, Line 521: cout << Indent(indent) << upd.key.ToString() << " "
: << RowChangeList(upd.cell).ToString(schema) << endl;
The old code (before removing the std:: prefixes) took care to align the <<
that begins each line. Could you do the same?
This comment applies elsewhere in this file too.
PS9, Line 607: bool found{false};
What's this? Why not "bool found = false;"?
PS9, Line 667: tablet
Replica.
Please make the same changes fo rthe other dump Descriptions.
PS9, Line 669: .AddOptionalParameter("rowset_index")
Resort this list of parameters alphabetically.
PS9, Line 679: tablet
Should now be 'replica'.
PS9, Line 721: tablet
Should be 'replica' now.
--
To view, visit http://gerrit.cloudera.org:8080/4305
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I1ec628b65613011d8c48b6239c13762276425966
Gerrit-PatchSet: 9
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Dinesh Bhat
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon
Gerrit-HasComments: Yes
[kudu-CR] tool: port kudu-fs dump, remove kudu-fs list, fs tool
Dinesh Bhat has posted comments on this change.
Change subject: tool: port kudu-fs_dump, remove kudu-fs_list, fs_tool
..
Patch Set 8:
(35 comments)
Thank you again Adar, addressed rev comments below, please see responses too
inline for few of the comments.
http://gerrit.cloudera.org:8080/#/c/4305/8/docs/release_notes.adoc
File docs/release_notes.adoc:
PS8, Line 71: local_repica
> local_replica
Done
http://gerrit.cloudera.org:8080/#/c/4305/5/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:
PS5, Line 517: ASSERT_STR_MATCHES(stdout, "Header:");
: ASSERT_STR_MATCHES(stdout, "1\\.1@1");
> Hmm, not exactly what I meant. What I mean is: you're already doing a subst
I see, sorry for misunderstanding earlier comment. Given that these are common
args for almost all commands I liked defining them in one place with an
intuitive variable name instead of spraying "--" args everywhere. I am keeping
them as-is where they are used multiple times but only changing them at places
where they are used only once. Lemme know.
http://gerrit.cloudera.org:8080/#/c/4305/8/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:
Line 20: #include
> Nit: should precede string.
Done
Line 215: const vector kLocalReplicaModeRegexes = {
> Shouldn't there be a dump mode in here? And something for "list all replica
Done
PS8, Line 594: string fs_paths = "--fs_wal_dir=" + kTestDir + " "
: "--fs_data_dirs=" + kTestDir;
> Same comment about partial substitution here.
Done
Line 596: LOG(INFO) < This was to help you debug, right? Can it be removed now?
thank you, good catch.
http://gerrit.cloudera.org:8080/#/c/4305/8/src/kudu/tools/tool_action_fs.cc
File src/kudu/tools/tool_action_fs.cc:
Line 19: #include "kudu/tools/tool_action_common.h"
> As before, this include doesn't belong up here.
Done
http://gerrit.cloudera.org:8080/#/c/4305/8/src/kudu/tools/tool_action_local_replica.cc
File src/kudu/tools/tool_action_local_replica.cc:
Line 28: #include "kudu/common/schema.h"
> Nit: should go after row*
Done
Line 33: #include "kudu/consensus/consensus.pb.h"
> Nit: shoudl go after consensus_meta.h.
Done
Line 53: #include "kudu/tablet/rowset_metadata.h"
> Nit: should come before tablet.h.
Done
Line 55: #include "kudu/tserver/tserver.pb.h"
> Nit: should go after tablet_copy_client.h.
Done
PS8, Line 58: #include "kudu/util/logging.h"
> Nit: should go after env_util.h.
Thank you, no doubt I did a pretty bad job here :)
PS8, Line 71: information(if any)
> Nit: information (if any)
Done
PS8, Line 114: DumpOptions
> I'd drop this struct altogether, because:
Agreed, done.
PS8, Line 286: static
> Nit: don't need this; the function is already in an anonymous namespace.
Fixed.
Line 287: const RowSetMetadata& rs_meta) {
> Nit: fix the indentation on this line.
Done
Line 292: std::cout << "Column block for column ID " << col_id;
> std::cout and std::endl are already in the 'using' blocks above, so you can
This became one indentation adjustment fun in the entire file :)
PS8, Line 318: const string* tablet_id = FindOrNull(context.required_args,
"tablet_id");
: if (tablet_id == nullptr) {
: LOG(INFO) << "No tablet_id specified, dumping all tablets:";
: }
> I understand the existing tool allowed this 'no tablet_id means dump all ta
Good catch, fixed.
PS8, Line 346: FsManager& fs_manager
> Google style frowns on passing arguments by ref. Your options are:
Cool, thanks. I changed them to pointer - pointer mainly because few following
callsites like FsManager::OpenBlock has non-const nature and also
TabletMetadata::Load takes a raw pointer, etc. I didn't really chase after why
Load function is taking a raw pointer to keep the scope of this change.
Line 381: std::cout << "\t" << tablet << std::endl;
> In this case, let's not bother with the leading tab. It'd be easier to pars
Done
PS8, Line 398: scoped_refptr(nullptr)
> I think this can just be "scoped_refptr()".
Interesting, done. I didn't really take this as an opportunity to tamper with
the original code, although part of the exercise was that.
PS8, Line 399: nullptr
> When passing a nullptr like this, consider the following style:
Very helpful, thank you.
PS8, Line 432: FsManager& fs_manager
> Const ref here too.
Done
PS8, Line 476: // NewDeltaIterator returns Status::OK() iff a new
DeltaIterator is created. Thus,
: // it's safe to have a gscoped_ptr take possesion of
'raw_iter' here.
: gscoped_ptr delta_iter(raw_iter);
> This is correct, but let's use unique_ptr now; this was written before we t
Done
PS8, Line 540: FsManager& fs_manager
> Let's change this to const ref.
Couldn't because of the reason mentioned above, eventually they call into
FSManager::OpenBlock which is
[kudu-CR] tool: port kudu-fs dump, remove kudu-fs list, fs tool
Kudu Jenkins has posted comments on this change. Change subject: tool: port kudu-fs_dump, remove kudu-fs_list, fs_tool .. Patch Set 9: Build Started http://104.196.14.100/job/kudu-gerrit/3362/ -- To view, visit http://gerrit.cloudera.org:8080/4305 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1ec628b65613011d8c48b6239c13762276425966 Gerrit-PatchSet: 9 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dinesh Bhat Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dinesh Bhat Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] tool: port kudu-fs dump, remove kudu-fs list, fs tool
Hello Adar Dembo, Todd Lipcon, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/4305 to look at the new patch set (#9). Change subject: tool: port kudu-fs_dump, remove kudu-fs_list, fs_tool .. tool: port kudu-fs_dump, remove kudu-fs_list, fs_tool This change ports fs_dump actions under 'kudu local_replica '. Additionally this has following re-organizations: - moved dump_cfile action under 'kudu fs dump cfile'. - kudu-fs_list tool has been removed altogether, but some of the functionalities are retained under 'local_replica' and 'fs dump' sub-actions. - fs_tool library is stripped off, and all those routines are in respective action files. Also added tests under kudu-tool-test to exercise each of these fs tools. Change-Id: I1ec628b65613011d8c48b6239c13762276425966 --- M docs/release_notes.adoc M src/kudu/integration-tests/master_migration-itest.cc M src/kudu/tablet/rowset_metadata.h M src/kudu/tools/CMakeLists.txt D src/kudu/tools/fs_tool.cc D src/kudu/tools/fs_tool.h M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool_action.h M src/kudu/tools/tool_action_fs.cc M src/kudu/tools/tool_action_local_replica.cc M src/kudu/tools/tool_action_tablet.cc 11 files changed, 806 insertions(+), 817 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/05/4305/9 -- To view, visit http://gerrit.cloudera.org:8080/4305 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I1ec628b65613011d8c48b6239c13762276425966 Gerrit-PatchSet: 9 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dinesh Bhat Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dinesh Bhat Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] tool: port kudu-fs dump, remove kudu-fs list, fs tool
Adar Dembo has posted comments on this change.
Change subject: tool: port kudu-fs_dump, remove kudu-fs_list, fs_tool
..
Patch Set 8:
(38 comments)
http://gerrit.cloudera.org:8080/#/c/4305/8/docs/release_notes.adoc
File docs/release_notes.adoc:
PS8, Line 71: local_repica
local_replica
http://gerrit.cloudera.org:8080/#/c/4305/5/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:
PS5, Line 517: ASSERT_STR_MATCHES(stdout, "Header:");
: ASSERT_STR_MATCHES(stdout, "1\\.1@1");
> Fixed, it was a blind copy-paste effect.
Hmm, not exactly what I meant. What I mean is: you're already doing a
substitute on L521; include the fs_wal_dir and fs_data_dirs substitution in
there rather than doing it out here.
That way the entire command line is more clear; don't need to look in two
different places.
http://gerrit.cloudera.org:8080/#/c/4305/8/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:
Line 20: #include
Nit: should precede string.
Line 215: const vector kLocalReplicaModeRegexes = {
Shouldn't there be a dump mode in here? And something for "list all replicas"?
PS8, Line 594: string fs_paths = "--fs_wal_dir=" + kTestDir + " "
: "--fs_data_dirs=" + kTestDir;
Same comment about partial substitution here.
Line 596: LOG(INFO)
[kudu-CR] tool: port kudu-fs dump, remove kudu-fs list, fs tool
Hello Adar Dembo, Todd Lipcon, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/4305 to look at the new patch set (#8). Change subject: tool: port kudu-fs_dump, remove kudu-fs_list, fs_tool .. tool: port kudu-fs_dump, remove kudu-fs_list, fs_tool This change ports fs_dump actions under 'kudu local_replica '. Additionally this has following re-organizations: - moved dump_cfile action under 'kudu fs dump cfile'. - kudu-fs_list tool has been removed altogether, but some of the functionalities are retained under 'local_replica' and 'fs dump' sub-actions. - fs_tool library is stripped off, and all those routines are in respective action files. Also added tests under kudu-tool-test to exercise each of these fs tools. Change-Id: I1ec628b65613011d8c48b6239c13762276425966 --- M docs/release_notes.adoc M src/kudu/integration-tests/master_migration-itest.cc M src/kudu/tablet/rowset_metadata.h M src/kudu/tools/CMakeLists.txt D src/kudu/tools/fs_tool.cc D src/kudu/tools/fs_tool.h M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool_action.h M src/kudu/tools/tool_action_fs.cc M src/kudu/tools/tool_action_local_replica.cc M src/kudu/tools/tool_action_tablet.cc 11 files changed, 854 insertions(+), 805 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/05/4305/8 -- To view, visit http://gerrit.cloudera.org:8080/4305 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I1ec628b65613011d8c48b6239c13762276425966 Gerrit-PatchSet: 8 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dinesh Bhat Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dinesh Bhat Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] tool: port kudu-fs dump, remove kudu-fs list, fs tool
Kudu Jenkins has posted comments on this change. Change subject: tool: port kudu-fs_dump, remove kudu-fs_list, fs_tool .. Patch Set 8: Build Started http://104.196.14.100/job/kudu-gerrit/3349/ -- To view, visit http://gerrit.cloudera.org:8080/4305 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1ec628b65613011d8c48b6239c13762276425966 Gerrit-PatchSet: 8 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dinesh Bhat Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dinesh Bhat Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] tool: port kudu-fs dump, remove kudu-fs list
Dinesh Bhat has posted comments on this change. Change subject: tool: port kudu-fs_dump, remove kudu-fs_list .. Patch Set 7: (1 comment) Thanks, please take a look at updated patch. http://gerrit.cloudera.org:8080/#/c/4305/5/src/kudu/tools/tool_action_common.h File src/kudu/tools/tool_action_common.h: Line 63 > I'd prefer option #2, as it strips away more of the existing code and (hope Done -- To view, visit http://gerrit.cloudera.org:8080/4305 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1ec628b65613011d8c48b6239c13762276425966 Gerrit-PatchSet: 7 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dinesh Bhat Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dinesh Bhat Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] tool: port kudu-fs dump, remove kudu-fs list
Kudu Jenkins has posted comments on this change. Change subject: tool: port kudu-fs_dump, remove kudu-fs_list .. Patch Set 7: Build Started http://104.196.14.100/job/kudu-gerrit/3348/ -- To view, visit http://gerrit.cloudera.org:8080/4305 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1ec628b65613011d8c48b6239c13762276425966 Gerrit-PatchSet: 7 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dinesh Bhat Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dinesh Bhat Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] tool: port kudu-fs dump, remove kudu-fs list
Hello Adar Dembo, Todd Lipcon, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/4305 to look at the new patch set (#7). Change subject: tool: port kudu-fs_dump, remove kudu-fs_list .. tool: port kudu-fs_dump, remove kudu-fs_list This change also moves dump_cfile action under 'kudu fs dump cfile'. kudu-fs_list tool has been removed altogether and a 'kudu fs dump tree' has been added to dump the filesystem tree. Majority of the kudu-fs_dump sub-commands which were relying on tablet id as required parameter have been moved under 'kudu tablet'. Also added tests under kudu-tool-test to exercise each of these fs tools. Change-Id: I1ec628b65613011d8c48b6239c13762276425966 --- M docs/release_notes.adoc M src/kudu/integration-tests/master_migration-itest.cc M src/kudu/tablet/rowset_metadata.h M src/kudu/tools/CMakeLists.txt D src/kudu/tools/fs_tool.cc D src/kudu/tools/fs_tool.h M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool_action.h M src/kudu/tools/tool_action_fs.cc M src/kudu/tools/tool_action_local_replica.cc M src/kudu/tools/tool_action_tablet.cc 11 files changed, 854 insertions(+), 805 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/05/4305/7 -- To view, visit http://gerrit.cloudera.org:8080/4305 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I1ec628b65613011d8c48b6239c13762276425966 Gerrit-PatchSet: 7 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dinesh Bhat Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dinesh Bhat Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] tool: port kudu-fs dump, remove kudu-fs list
Adar Dembo has posted comments on this change.
Change subject: tool: port kudu-fs_dump, remove kudu-fs_list
..
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4305/5/src/kudu/tools/tool_action_common.h
File src/kudu/tools/tool_action_common.h:
Line 63: class FsTool {
> Yeah, I couldn't be more convinced here; though I have a follow up Qn about
I'd prefer option #2, as it strips away more of the existing code and
(hopefully) exposes opportunities to remove old cruft.
--
To view, visit http://gerrit.cloudera.org:8080/4305
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I1ec628b65613011d8c48b6239c13762276425966
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Dinesh Bhat
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon
Gerrit-HasComments: Yes
[kudu-CR] tool: port kudu-fs dump, remove kudu-fs list
Dinesh Bhat has posted comments on this change.
Change subject: tool: port kudu-fs_dump, remove kudu-fs_list
..
Patch Set 5:
(22 comments)
TFTR Adar, except for one follow-up Qn about cleanup in common header, I have
addressed all cleanups below. Please lemme know if I can get one more round of
eyeballs after that.
http://gerrit.cloudera.org:8080/#/c/4305/5/docs/release_notes.adoc
File docs/release_notes.adoc:
PS5, Line 49: - The `cfile-dump` tool has been removed. The same functionality
is now
: implemented as `kudu fs cfile_dump`.
> Update this.
Done
PS5, Line 58: The `kudu-fs_list` tool has been removed as it was replicating
most of the
: functionalities already available under 'kudu fs dump'.
> This should be reworded now that some functionality was preserved, right?
Done
http://gerrit.cloudera.org:8080/#/c/4305/5/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:
Line 202: const vector kFsDumpModeRegexes = {
> Need to add uuid to this list.
Done
PS5, Line 501: const string kTestTablet = "test-tablet";
: const string kTestTable = "test-table";
> Nit: can you add "Id" as a suffix to these variable names, so it's clear th
Done
PS5, Line 517: string fs_paths = Substitute("--fs_wal_dir=$0
--fs_data_dirs=$1",
:kTestDir, kTestDir);
> Nit: not sure what this partial substitution buys you; fs_paths is only use
Fixed, it was a blind copy-paste effect.
Line 534: StripWhiteSpace(&debug_str);
> What whitespace is being stripped here?
yeah probably nothing, fixed.
PS5, Line 568: std::ostringstream
> Nit: add this to using statements at the top of the file. Should also add t
Done
Line 570: ASSERT_STR_MATCHES(stdout, tree_out.str());
> We can't do ASSERT_EQ() here?
cool, fixed.
Line 582: unique_ptr harness(new TabletHarness(kSchemaWithIds,
opts));
> Can't we just declare the TabletHarness on the stack and avoid the unique_p
Done
Line 583: CHECK_OK(harness->Create(true));
> These CHECK_OK() statements should be ASSERT_OK() instead. CHECK_OK() is on
I see, I thought ASSERT is for debugs and CHECK for all build types, I was
prolly confused between logging vs status routines. Thank you for noting that.
Also, would you mind explaining me co-relation between the thread and CHECK_OK,
I didn't understand that part.
PS5, Line 587: gscoped_ptr writer(
: new LocalTabletWriter(harness->tablet().get(), &kSchema));
: gscoped_ptr row(new
KuduPartialRow(&kSchemaWithIds));
> Likewise, can't we declare these on the stack?
Done
PS5, Line 606: vector rows = strings::Split(stdout, "\n",
strings::SkipEmpty());
> Why not use RunActionStdoutLines to get this?
Done
PS5, Line 621: if (stdout.find("No rowsets found") == string::npos) {
> AFAICT this test is deterministic, so will we get "No rowsets found" or not
Done, yes it is deterministic.
http://gerrit.cloudera.org:8080/#/c/4305/5/src/kudu/tools/tool_action_common.h
File src/kudu/tools/tool_action_common.h:
Line 63: class FsTool {
> My mistake, I wasn't clear when I asked you to remove fs_tool. I didn't mea
Yeah, I couldn't be more convinced here; though I have a follow up Qn about
this cleanup : 1) we could either keep all these routines and just strip off
FsTool, and have these routines take fs_manager as argument or 2) We could move
their functionatlities to respective callers themselves - i.e for example
DumpRowSet inside action_local_replica.cc will do everything inside it's body.
Let me know what you think.
http://gerrit.cloudera.org:8080/#/c/4305/5/src/kudu/tools/tool_action_fs.cc
File src/kudu/tools/tool_action_fs.cc:
PS5, Line 138: Print
> Nit: Change to "Dump" to be consistent with other dump actions.
Done
http://gerrit.cloudera.org:8080/#/c/4305/5/src/kudu/tools/tool_action_tablet.cc
File src/kudu/tools/tool_action_tablet.cc:
Line 19: #include "kudu/tools/tool_action_common.h"
> This actually belongs where it was previously. The top-most include is rese
Huh, thanks for catching that, it probably takes another year for me to digest
all of that coding guidelines :)
PS5, Line 331: dump_wals
> When you add a dump submode, move this into it too.
Done
PS5, Line 343: list_blocks
> How about "dump_block_ids"? Then you can add it to a dump submode, rename t
Done
Line 351: ActionBuilder("list_tablets", &ListAllTablets)
> Nit: let's just call this one "list", because "kudu tablet list" makes intu
Done
PS5, Line 385: rowset
> Nit: rowset contents
Done
PS5, Line 385: tablet
> Nit: "of a tablet"
Done
PS5, Line 399: .AddAction(std::move(list_blocks))
: .AddAction(std::move(list_tablets))
: .AddAction(std::move(dump_blocks))
: .AddAction(std::move(dump_data))
: .AddAction(std::move(dump_meta))
[kudu-CR] tool: port kudu-fs dump, remove kudu-fs list
Kudu Jenkins has posted comments on this change. Change subject: tool: port kudu-fs_dump, remove kudu-fs_list .. Patch Set 6: Build Started http://104.196.14.100/job/kudu-gerrit/3338/ -- To view, visit http://gerrit.cloudera.org:8080/4305 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1ec628b65613011d8c48b6239c13762276425966 Gerrit-PatchSet: 6 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dinesh Bhat Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dinesh Bhat Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] tool: port kudu-fs dump, remove kudu-fs list
Hello Adar Dembo, Todd Lipcon, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/4305 to look at the new patch set (#6). Change subject: tool: port kudu-fs_dump, remove kudu-fs_list .. tool: port kudu-fs_dump, remove kudu-fs_list This change also moves dump_cfile action under 'kudu fs dump cfile'. kudu-fs_list tool has been removed altogether and a 'kudu fs dump tree' has been added to dump the filesystem tree. Majority of the kudu-fs_dump sub-commands which were relying on tablet id as required parameter have been moved under 'kudu tablet'. Also added tests under kudu-tool-test to exercise each of these fs tools. Change-Id: I1ec628b65613011d8c48b6239c13762276425966 --- M docs/release_notes.adoc M src/kudu/integration-tests/master_migration-itest.cc M src/kudu/tools/CMakeLists.txt D src/kudu/tools/fs_tool.cc D src/kudu/tools/fs_tool.h M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool_action.h M src/kudu/tools/tool_action_common.cc M src/kudu/tools/tool_action_common.h M src/kudu/tools/tool_action_fs.cc M src/kudu/tools/tool_action_local_replica.cc M src/kudu/tools/tool_action_tablet.cc 12 files changed, 1,141 insertions(+), 797 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/05/4305/6 -- To view, visit http://gerrit.cloudera.org:8080/4305 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I1ec628b65613011d8c48b6239c13762276425966 Gerrit-PatchSet: 6 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dinesh Bhat Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dinesh Bhat Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] tool: port kudu-fs dump, remove kudu-fs list
Adar Dembo has posted comments on this change.
Change subject: tool: port kudu-fs_dump, remove kudu-fs_list
..
Patch Set 5:
(22 comments)
http://gerrit.cloudera.org:8080/#/c/4305/5/docs/release_notes.adoc
File docs/release_notes.adoc:
PS5, Line 49: - The `cfile-dump` tool has been removed. The same functionality
is now
: implemented as `kudu fs cfile_dump`.
Update this.
PS5, Line 58: The `kudu-fs_list` tool has been removed as it was replicating
most of the
: functionalities already available under 'kudu fs dump'.
This should be reworded now that some functionality was preserved, right?
http://gerrit.cloudera.org:8080/#/c/4305/5/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:
Line 202: const vector kFsDumpModeRegexes = {
Need to add uuid to this list.
PS5, Line 501: const string kTestTablet = "test-tablet";
: const string kTestTable = "test-table";
Nit: can you add "Id" as a suffix to these variable names, so it's clear that
it's a table ID (as opposed to the table name, below)?
In the other new tests too.
PS5, Line 517: string fs_paths = Substitute("--fs_wal_dir=$0
--fs_data_dirs=$1",
:kTestDir, kTestDir);
Nit: not sure what this partial substitution buys you; fs_paths is only used in
one place here and in the next test.
Line 534: StripWhiteSpace(&debug_str);
What whitespace is being stripped here?
PS5, Line 568: std::ostringstream
Nit: add this to using statements at the top of the file. Should also add the
right include too.
Line 570: ASSERT_STR_MATCHES(stdout, tree_out.str());
We can't do ASSERT_EQ() here?
Line 582: unique_ptr harness(new TabletHarness(kSchemaWithIds,
opts));
Can't we just declare the TabletHarness on the stack and avoid the unique_ptr?
Line 583: CHECK_OK(harness->Create(true));
These CHECK_OK() statements should be ASSERT_OK() instead. CHECK_OK() is only
needed when being called on a separate thread in a unit test (here we're on the
main thread).
PS5, Line 587: gscoped_ptr writer(
: new LocalTabletWriter(harness->tablet().get(), &kSchema));
: gscoped_ptr row(new
KuduPartialRow(&kSchemaWithIds));
Likewise, can't we declare these on the stack?
PS5, Line 606: vector rows = strings::Split(stdout, "\n",
strings::SkipEmpty());
Why not use RunActionStdoutLines to get this?
PS5, Line 621: if (stdout.find("No rowsets found") == string::npos) {
AFAICT this test is deterministic, so will we get "No rowsets found" or not?
Given that the bulk of the interesting testing is when we don't get that error,
we should do whatever we need to do up-front (i.e. write more data, or flush,
or whatever) so that we can convert this into an ASSERT instead.
Below too.
http://gerrit.cloudera.org:8080/#/c/4305/5/src/kudu/tools/tool_action_common.h
File src/kudu/tools/tool_action_common.h:
Line 63: class FsTool {
My mistake, I wasn't clear when I asked you to remove fs_tool. I didn't mean
"move FsTool from the fs_tool.* code files into tool_action.*"; I meant "remove
the FsTool abstraction altogether and replace it with stateless functions".
Given the simplicity of what we're trying to do, I don't think the abstraction
buys us anything, and I'd like to use this opportunity (of writing a new tool)
to get rid of old abstractions and assumptions.
http://gerrit.cloudera.org:8080/#/c/4305/5/src/kudu/tools/tool_action_fs.cc
File src/kudu/tools/tool_action_fs.cc:
PS5, Line 138: Print
Nit: Change to "Dump" to be consistent with other dump actions.
http://gerrit.cloudera.org:8080/#/c/4305/5/src/kudu/tools/tool_action_tablet.cc
File src/kudu/tools/tool_action_tablet.cc:
Line 19: #include "kudu/tools/tool_action_common.h"
This actually belongs where it was previously. The top-most include is reserved
for the primary header of this compilation unit.
See
https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes
for details.
PS5, Line 331: dump_wals
When you add a dump submode, move this into it too.
PS5, Line 343: list_blocks
How about "dump_block_ids"? Then you can add it to a dump submode, rename the
associated function "DumpTabletBlockIds", and change the description to "Dump
the IDs of all blocks belonging to a tablet".
Line 351: ActionBuilder("list_tablets", &ListAllTablets)
Nit: let's just call this one "list", because "kudu tablet list" makes
intuitive sense.
PS5, Line 385: tablet
Nit: "of a tablet"
Above too.
PS5, Line 385: rowset
Nit: rowset contents
PS5, Line 399: .AddAction(std::move(list_blocks))
: .AddAction(std::move(list_tablets))
: .AddAction(std::move(dump_blocks))
: .AddAction(std::move(dump_data))
: .AddAction(std::move(dump_meta))
: .AddAction(std::move(dump_rowset))
Ho
[kudu-CR] tool: port kudu-fs dump, remove kudu-fs list
Kudu Jenkins has posted comments on this change. Change subject: tool: port kudu-fs_dump, remove kudu-fs_list .. Patch Set 5: Build Started http://104.196.14.100/job/kudu-gerrit/3304/ -- To view, visit http://gerrit.cloudera.org:8080/4305 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1ec628b65613011d8c48b6239c13762276425966 Gerrit-PatchSet: 5 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dinesh Bhat Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dinesh Bhat Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] tool: port kudu-fs dump, remove kudu-fs list
Hello Adar Dembo, Todd Lipcon, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/4305 to look at the new patch set (#5). Change subject: tool: port kudu-fs_dump, remove kudu-fs_list .. tool: port kudu-fs_dump, remove kudu-fs_list This change also moves dump_cfile action under 'kudu fs dump cfile'. kudu-fs_list tool has been removed altogether and a 'kudu fs dump tree' has been added to dump the filesystem tree. Majority of the kudu-fs_dump sub-commands which were relying on tablet id as required parameter have been moved under 'kudu tablet'. Also added tests under kudu-tool-test to exercise each of these fs tools. Change-Id: I1ec628b65613011d8c48b6239c13762276425966 --- M docs/release_notes.adoc M src/kudu/integration-tests/master_migration-itest.cc M src/kudu/tools/CMakeLists.txt D src/kudu/tools/fs_dump-tool.cc D src/kudu/tools/fs_list-tool.cc D src/kudu/tools/fs_tool.cc D src/kudu/tools/fs_tool.h M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool_action.h M src/kudu/tools/tool_action_common.cc M src/kudu/tools/tool_action_common.h M src/kudu/tools/tool_action_fs.cc M src/kudu/tools/tool_action_tablet.cc 13 files changed, 1,117 insertions(+), 1,151 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/05/4305/5 -- To view, visit http://gerrit.cloudera.org:8080/4305 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I1ec628b65613011d8c48b6239c13762276425966 Gerrit-PatchSet: 5 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dinesh Bhat Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dinesh Bhat Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] tool: port kudu-fs dump, remove kudu-fs list (WIP)
Kudu Jenkins has posted comments on this change. Change subject: tool: port kudu-fs_dump, remove kudu-fs_list (WIP) .. Patch Set 4: Build Started http://104.196.14.100/job/kudu-gerrit/3279/ -- To view, visit http://gerrit.cloudera.org:8080/4305 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1ec628b65613011d8c48b6239c13762276425966 Gerrit-PatchSet: 4 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dinesh Bhat Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dinesh Bhat Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] tool: port kudu-fs dump, remove kudu-fs list (WIP)
Hello Adar Dembo, Todd Lipcon, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/4305 to look at the new patch set (#4). Change subject: tool: port kudu-fs_dump, remove kudu-fs_list (WIP) .. tool: port kudu-fs_dump, remove kudu-fs_list (WIP) This change also moves dump_cfile action under 'kudu fs dump cfile'. kudu-fs_list tool has been removed altogether and a 'kudu fs dump tree' has been added to dump the filesystem tree. Majority of the kudu-fs_dump sub-commands which were relying on tablet id as required parameter have been moved under 'kudu tablet'. Also added tests under kudu-tool-test to exercise each of these fs tools. Change-Id: I1ec628b65613011d8c48b6239c13762276425966 --- M docs/release_notes.adoc M src/kudu/integration-tests/master_migration-itest.cc M src/kudu/tools/CMakeLists.txt D src/kudu/tools/fs_dump-tool.cc D src/kudu/tools/fs_list-tool.cc D src/kudu/tools/fs_tool.cc D src/kudu/tools/fs_tool.h M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool_action.h M src/kudu/tools/tool_action_common.cc M src/kudu/tools/tool_action_common.h M src/kudu/tools/tool_action_fs.cc M src/kudu/tools/tool_action_tablet.cc 13 files changed, 1,117 insertions(+), 1,151 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/05/4305/4 -- To view, visit http://gerrit.cloudera.org:8080/4305 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I1ec628b65613011d8c48b6239c13762276425966 Gerrit-PatchSet: 4 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dinesh Bhat Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dinesh Bhat Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] tool: port kudu-fs dump, remove kudu-fs list (WIP)
Kudu Jenkins has posted comments on this change. Change subject: tool: port kudu-fs_dump, remove kudu-fs_list (WIP) .. Patch Set 3: Build Started http://104.196.14.100/job/kudu-gerrit/3253/ -- To view, visit http://gerrit.cloudera.org:8080/4305 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1ec628b65613011d8c48b6239c13762276425966 Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dinesh Bhat Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dinesh Bhat Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] tool: port kudu-fs dump, remove kudu-fs list (WIP)
Hello Adar Dembo, Todd Lipcon, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/4305 to look at the new patch set (#3). Change subject: tool: port kudu-fs_dump, remove kudu-fs_list (WIP) .. tool: port kudu-fs_dump, remove kudu-fs_list (WIP) This change also moves dump_cfile action under 'kudu fs dump cfile'. kudu-fs_list tool has been removed altogether and a 'kudu fs dump tree' has been added to dump the filesystem tree. Majority of the kudu-fs_dump sub-commands which were relying on tablet id as required parameter have been moved under 'kudu tablet'. Also added tests under kudu-tool-test to exercise each of these fs tools. Change-Id: I1ec628b65613011d8c48b6239c13762276425966 --- M docs/release_notes.adoc M src/kudu/tools/CMakeLists.txt D src/kudu/tools/fs_dump-tool.cc D src/kudu/tools/fs_list-tool.cc D src/kudu/tools/fs_tool.cc D src/kudu/tools/fs_tool.h M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool_action.h M src/kudu/tools/tool_action_common.cc M src/kudu/tools/tool_action_common.h M src/kudu/tools/tool_action_fs.cc M src/kudu/tools/tool_action_tablet.cc 12 files changed, 1,087 insertions(+), 1,146 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/05/4305/3 -- To view, visit http://gerrit.cloudera.org:8080/4305 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I1ec628b65613011d8c48b6239c13762276425966 Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dinesh Bhat Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dinesh Bhat Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] tool: port kudu-fs dump, remove kudu-fs list (WIP)
Dinesh Bhat has posted comments on this change.
Change subject: tool: port kudu-fs_dump, remove kudu-fs_list (WIP)
..
Patch Set 1:
(7 comments)
TFTR Adar, updated the patch, please re-review, also added some more tests. I
think I can keep adding some more test exercises as you review them.
http://gerrit.cloudera.org:8080/#/c/4305/1/docs/release_notes.adoc
File docs/release_notes.adoc:
Line 58: - The `kudu-fs_list` tool has been removed as it was replicating most
of the
> As I mentioned in HipChat the other day, I think the LIST_BLOCKS functional
Added that back, although I have kept some of the sub commands like
tree/list_tablets as well now since it's useful to pretty print them instead of
relying on 'find'. Also please find some routines like ListLogSegments etc
which I think could be removed. I will do so after taking some confirmation
from your end.
PS1, Line 59: options
> Nit: change to 'functionality'
Done
http://gerrit.cloudera.org:8080/#/c/4305/1/src/kudu/tools/CMakeLists.txt
File src/kudu/tools/CMakeLists.txt:
PS1, Line 55: add_library(fs_tool fs_tool.cc)
: target_link_libraries(fs_tool
: gutil
: kudu_common
: server_common
: consensus
: tablet)
> Can you remove fs_tool altogether and move the needed functionality into th
I have done this, but not exactly as you described. I moved pretty much
everything under tool_action_common(as opposed to moving just common routines),
reason being I thought these subcommands may go through another round of
re-shuffle after feedbacks from dev/users, we could start reorganizing them at
that point. For now, FsTool simply moved from fs_tool library to
tool_action_common.* and got rid of fs_tool library as such.
http://gerrit.cloudera.org:8080/#/c/4305/1/src/kudu/tools/tool_action_fs.cc
File src/kudu/tools/tool_action_fs.cc:
PS1, Line 187: unique_ptr dump_fs_blocks =
: ActionBuilder("tablet_blocks", &DumpFsTabletBlocks)
: .Description("Dump the data of tablet")
: .AddRequiredParameter({ "tablet_id", "tablet identifier" })
: .AddOptionalParameter("fs_wal_dir")
: .AddOptionalParameter("fs_data_dirs")
: .Build();
:
: unique_ptr dump_fs_data =
: ActionBuilder("tablet_data", &DumpFsTabletData)
: .Description("Dump the data of tablet")
: .AddRequiredParameter({ "tablet_id", "tablet identifier" })
: .AddOptionalParameter("fs_wal_dir")
: .AddOptionalParameter("fs_data_dirs")
: .Build();
:
: unique_ptr dump_fs_meta =
: ActionBuilder("tablet_meta", &DumpFsTabletMeta)
: .Description("Dump the metadata of tablet")
: .AddRequiredParameter({ "tablet_id", "tablet identifier" })
: .AddOptionalParameter("fs_wal_dir")
: .AddOptionalParameter("fs_data_dirs")
: .Build();
:
: unique_ptr dump_fs_rowset =
: ActionBuilder("rowset", &DumpFsTabletRowSet)
: .Description("Dump the rowset of tablet")
: .AddRequiredParameter({ "tablet_id", "tablet identifier" })
: .AddRequiredParameter({ "rowset_index", "rowset index" })
: .AddOptionalParameter("fs_wal_dir")
: .AddOptionalParameter("fs_data_dirs")
: .Build();
> These are all scoped to a tablet, so I think they should be in the tablet m
Done, kept the dump_wals as-is since it seemed to be taking tablet_id as param.
PS1, Line 220: unique_ptr dump_tree =
: ActionBuilder("tree", &DumpFsTree)
: .Description("Dump the tree of Kudu filesystem")
: .AddOptionalParameter("fs_wal_dir")
: .AddOptionalParameter("fs_data_dirs")
: .Build();
> Let's drop this one altogether since basic UNIX tools like find can do the
Hmmm, I kept this only for pretty printing purposes, kinda looks nice with
hierarchies enforced by indentations.
PS1, Line 237: .AddAction(std::move(dump_fs_blocks))
: .AddAction(std::move(dump_fs_data))
: .AddAction(std::move(dump_fs_meta))
: .AddAction(std::move(dump_fs_rowset))
> These are all scoped to the 'fs' mode, so let's drop the fs infix.
Done
PS1, Line 242: print_uuid
> For consistency, let's change this to dump_uuid. You'll probably need to up
Done
--
To view, visit http://gerrit.cloudera.org:8080/4305
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I1ec628b65613011d8c48b6239c137622
[kudu-CR] tool: port kudu-fs dump, remove kudu-fs list (WIP)
Kudu Jenkins has posted comments on this change. Change subject: tool: port kudu-fs_dump, remove kudu-fs_list (WIP) .. Patch Set 2: Build Started http://104.196.14.100/job/kudu-gerrit/3249/ -- To view, visit http://gerrit.cloudera.org:8080/4305 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1ec628b65613011d8c48b6239c13762276425966 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dinesh Bhat Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dinesh Bhat Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] tool: port kudu-fs dump, remove kudu-fs list (WIP)
Hello Adar Dembo, Todd Lipcon, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/4305 to look at the new patch set (#2). Change subject: tool: port kudu-fs_dump, remove kudu-fs_list (WIP) .. tool: port kudu-fs_dump, remove kudu-fs_list (WIP) This change also moves dump_cfile action under 'kudu fs dump cfile'. kudu-fs_list tool has been removed altogether and a 'kudu fs dump tree' has been added to dump the filesystem tree. Majority of the kudu-fs_dump sub-commands which were relying on tablet id as required parameter have been moved under 'kudu tablet'. Also added tests under kudu-tool-test to exercise each of these fs tools. Change-Id: I1ec628b65613011d8c48b6239c13762276425966 --- M docs/release_notes.adoc M src/kudu/tools/CMakeLists.txt D src/kudu/tools/fs_dump-tool.cc D src/kudu/tools/fs_list-tool.cc D src/kudu/tools/fs_tool.cc D src/kudu/tools/fs_tool.h M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool_action.h M src/kudu/tools/tool_action_common.cc M src/kudu/tools/tool_action_common.h M src/kudu/tools/tool_action_fs.cc M src/kudu/tools/tool_action_tablet.cc 12 files changed, 1,081 insertions(+), 1,146 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/05/4305/2 -- To view, visit http://gerrit.cloudera.org:8080/4305 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I1ec628b65613011d8c48b6239c13762276425966 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dinesh Bhat Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] tool: port kudu-fs dump, remove kudu-fs list (WIP)
Adar Dembo has posted comments on this change.
Change subject: tool: port kudu-fs_dump, remove kudu-fs_list (WIP)
..
Patch Set 1:
(7 comments)
Just did a quick first pass.
http://gerrit.cloudera.org:8080/#/c/4305/1/docs/release_notes.adoc
File docs/release_notes.adoc:
Line 58: - The `kudu-fs_list` tool has been removed as it was replicating most
of the
As I mentioned in HipChat the other day, I think the LIST_BLOCKS functionality
in kudu-fs_list should be preserved, because there isn't a 1-1 relationship
between files and blocks, and so finding all blocks (scoped to a tablet or in
total) isn't trivial.
PS1, Line 59: options
Nit: change to 'functionality'
http://gerrit.cloudera.org:8080/#/c/4305/1/src/kudu/tools/CMakeLists.txt
File src/kudu/tools/CMakeLists.txt:
PS1, Line 55: add_library(fs_tool fs_tool.cc)
: target_link_libraries(fs_tool
: gutil
: kudu_common
: server_common
: consensus
: tablet)
Can you remove fs_tool altogether and move the needed functionality into the
tool_action_* code files? Especially with the removal of fs_list-tool there's
functionality in fs_tool that's no longer needed.
http://gerrit.cloudera.org:8080/#/c/4305/1/src/kudu/tools/tool_action_fs.cc
File src/kudu/tools/tool_action_fs.cc:
PS1, Line 187: unique_ptr dump_fs_blocks =
: ActionBuilder("tablet_blocks", &DumpFsTabletBlocks)
: .Description("Dump the data of tablet")
: .AddRequiredParameter({ "tablet_id", "tablet identifier" })
: .AddOptionalParameter("fs_wal_dir")
: .AddOptionalParameter("fs_data_dirs")
: .Build();
:
: unique_ptr dump_fs_data =
: ActionBuilder("tablet_data", &DumpFsTabletData)
: .Description("Dump the data of tablet")
: .AddRequiredParameter({ "tablet_id", "tablet identifier" })
: .AddOptionalParameter("fs_wal_dir")
: .AddOptionalParameter("fs_data_dirs")
: .Build();
:
: unique_ptr dump_fs_meta =
: ActionBuilder("tablet_meta", &DumpFsTabletMeta)
: .Description("Dump the metadata of tablet")
: .AddRequiredParameter({ "tablet_id", "tablet identifier" })
: .AddOptionalParameter("fs_wal_dir")
: .AddOptionalParameter("fs_data_dirs")
: .Build();
:
: unique_ptr dump_fs_rowset =
: ActionBuilder("rowset", &DumpFsTabletRowSet)
: .Description("Dump the rowset of tablet")
: .AddRequiredParameter({ "tablet_id", "tablet identifier" })
: .AddRequiredParameter({ "rowset_index", "rowset index" })
: .AddOptionalParameter("fs_wal_dir")
: .AddOptionalParameter("fs_data_dirs")
: .Build();
These are all scoped to a tablet, so I think they should be in the tablet mode,
not fs. Given that, probably makes sense to leave dump_cfile as it was in fs
and put the dump mode in tablet. You can move 'dump_wals' already in tablet
into the new dump mode too.
PS1, Line 220: unique_ptr dump_tree =
: ActionBuilder("tree", &DumpFsTree)
: .Description("Dump the tree of Kudu filesystem")
: .AddOptionalParameter("fs_wal_dir")
: .AddOptionalParameter("fs_data_dirs")
: .Build();
Let's drop this one altogether since basic UNIX tools like find can do the same
thing.
PS1, Line 237: .AddAction(std::move(dump_fs_blocks))
: .AddAction(std::move(dump_fs_data))
: .AddAction(std::move(dump_fs_meta))
: .AddAction(std::move(dump_fs_rowset))
These are all scoped to the 'fs' mode, so let's drop the fs infix.
PS1, Line 242: print_uuid
For consistency, let's change this to dump_uuid. You'll probably need to update
several files.
--
To view, visit http://gerrit.cloudera.org:8080/4305
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I1ec628b65613011d8c48b6239c13762276425966
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon
Gerrit-HasComments: Yes
[kudu-CR] tool: port kudu-fs dump, remove kudu-fs list (WIP)
Hello Adar Dembo, Todd Lipcon, I'd like you to do a code review. Please visit http://gerrit.cloudera.org:8080/4305 to review the following change. Change subject: tool: port kudu-fs_dump, remove kudu-fs_list (WIP) .. tool: port kudu-fs_dump, remove kudu-fs_list (WIP) This change also moves dump_cfile action under 'kudu fs dump cfile'. kudu-fs_list tool has been removed altogether and a 'kudu fs dump tree' has been added to dump the filesystem tree. Also added tests under kudu-tool-test to exercise each of these fs tools. Change-Id: I1ec628b65613011d8c48b6239c13762276425966 --- M docs/release_notes.adoc M src/kudu/tools/CMakeLists.txt M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool_action_fs.cc 4 files changed, 223 insertions(+), 30 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/05/4305/1 -- To view, visit http://gerrit.cloudera.org:8080/4305 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I1ec628b65613011d8c48b6239c13762276425966 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dinesh Bhat Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Todd Lipcon
[kudu-CR] tool: port kudu-fs dump, remove kudu-fs list (WIP)
Kudu Jenkins has posted comments on this change. Change subject: tool: port kudu-fs_dump, remove kudu-fs_list (WIP) .. Patch Set 1: Build Started http://104.196.14.100/job/kudu-gerrit/3213/ -- To view, visit http://gerrit.cloudera.org:8080/4305 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1ec628b65613011d8c48b6239c13762276425966 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dinesh Bhat Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
