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 <sstream> > Nit: should precede string. Done Line 215: const vector<string> 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) <<fs_paths; > 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<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. 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<DeltaIterator> 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 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 Removed. 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 Done PS8, Line 673: PrintTabletMeta > Nit: let's rename this to DumpTabletMeta to be consistent with the other fu Done Line 679: static unique_ptr<Mode> BuildDumpMode() { > Nit: move this into the anonymous namespace above and then you won't need ' Done 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." 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: 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
