Dinesh Bhat has posted comments on this change.

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

Patch Set 8:


Thank you again Adar, addressed rev comments below, please see responses too 
inline for few of the comments.

File docs/release_notes.adoc:

PS8, Line 71: local_repica
> local_replica

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.

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

Line 20: #include <sstream>
> Nit: should precede string.

Line 215:     const vector<string> kLocalReplicaModeRegexes = {
> Shouldn't there be a dump mode in here? And something for "list all replica

PS8, Line 594:   string fs_paths = "--fs_wal_dir=" + kTestDir + " "
             :       "--fs_data_dirs=" + kTestDir;
> Same comment about partial substitution here.

Line 596:   LOG(INFO) <<fs_paths;
> This was to help you debug, right? Can it be removed now?
thank you, good catch.

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.

File src/kudu/tools/tool_action_local_replica.cc:

Line 28: #include "kudu/common/schema.h"
> Nit: should go after row*

Line 33: #include "kudu/consensus/consensus.pb.h"
> Nit: shoudl go after consensus_meta.h.

Line 53: #include "kudu/tablet/rowset_metadata.h"
> Nit: should come before tablet.h.

Line 55: #include "kudu/tserver/tserver.pb.h"
> Nit: should go after tablet_copy_client.h.

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)

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.

Line 287:                                   const RowSetMetadata& rs_meta) {
> Nit: fix the indentation on this line.

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, 
             :   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

PS8, Line 398: scoped_refptr<server::Clock>(nullptr)
> I think this can just be "scoped_refptr<server::Clock>()".
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.

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<DeltaIterator> delta_iter(raw_iter);
> This is correct, but let's use unique_ptr now; this was written before we t

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 non-const. I am not sure const_cast is possible 
later or not, but that sounds incorrect/ugly.

PS8, Line 608:   DumpOptions opts;
             :   opts.nrows = FLAGS_nrows;
             :   opts.metadata_only = FLAGS_metadata_only;
> Move this to just before the loop on L625 (no point in reconstructing it wi
I got rid of them altogether as per your suggestion above.

PS8, Line 621:   Schema schema = meta->schema();
> I don't see us using this anywhere, and it's not clear why we want to make 

Line 640:   uint32_t rowset_idx;
> Let's parse this as a 64-bit integer; that's how large it can be in RowSetM
Very good catch, fixed.

PS8, Line 648:   DumpOptions opts;
             :   opts.nrows = FLAGS_nrows;
             :   opts.metadata_only = FLAGS_metadata_only;
> Nit: move this to just before L657, as it's not needed before.
Removed them.

PS8, Line 667:   FsManagerOpts fs_opts;
             :   fs_opts.read_only = true;
             :   FsManager fs_manager(Env::Default(), fs_opts);
             :   RETURN_NOT_OK(fs_manager.Open());
> Given how often this is repeated, consider decomposing it into a local func
Done, although I am keeping the return type as Status with FsManager::Open 
retunring status etc which callers can leverage directly, and making the 
function take a unique_ptr as arg.

Line 673:   CHECK_OK(PrintTabletMeta(fs_manager, tablet_id, 0));
> Why CHECK_OK? Shouldn't we RETURN_NOT_OK() on this? Or just "return PrintTa

PS8, Line 673: PrintTabletMeta
> Nit: let's rename this to DumpTabletMeta to be consistent with the other fu

Line 679: static unique_ptr<Mode> BuildDumpMode() {
> Nit: move this into the anonymous namespace above and then you won't need '

PS8, Line 682:       .Description("Dump the data of a tablet")
> This needs to be differentiated from dump_data in some way. Presumably this
Very interesting catch ! it turns out dump blocks was nothing but dumping all 
rowsets, I removed dump blocks and retained rowset with an optional param 
rowset_index, if nothing specified, dumping all rowsets. Added a small test to 
check negative case too.

PS8, Line 786: tablets
> Nit: "Kudu replicas in the local filesystem."

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 <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