Dinesh Bhat has posted comments on this change.

Change subject: tool: port kudu-fs_dump, remove kudu-fs_list

Patch Set 5:


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.

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?

File src/kudu/tools/kudu-tool-test.cc:

Line 202:     const vector<string> 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 th

PS5, Line 517:   string fs_paths = Substitute("--fs_wal_dir=$0 
             :                                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

Line 570:   ASSERT_STR_MATCHES(stdout, tree_out.str());
> We can't do ASSERT_EQ() here?
cool, fixed.

Line 582:   unique_ptr<TabletHarness> harness(new TabletHarness(kSchemaWithIds, 
> Can't we just declare the TabletHarness on the stack and avoid the unique_p

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<LocalTabletWriter> writer(
             :       new LocalTabletWriter(harness->tablet().get(), &kSchema));
             :   gscoped_ptr<KuduPartialRow> row(new 
> Likewise, can't we declare these on the stack?

PS5, Line 606:     vector<string> rows = strings::Split(stdout, "\n", 
> 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
Done, yes it is deterministic.

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.

File src/kudu/tools/tool_action_fs.cc:

PS5, Line 138: Print
> Nit: Change to "Dump" to be consistent with other dump actions.

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.

PS5, Line 343: list_blocks
> How about "dump_block_ids"? Then you can add it to a dump submode, rename t

Line 351:       ActionBuilder("list_tablets", &ListAllTablets)
> Nit: let's just call this one "list", because "kudu tablet list" makes intu

PS5, Line 385: rowset
> Nit: rowset contents

PS5, Line 385: tablet
> Nit: "of a tablet"

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))
> How about a 'dump' mode to consolidate? Maybe even a 'list' mode, since "du

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 <din...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <a...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <din...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <t...@apache.org>
Gerrit-HasComments: Yes

Reply via email to