Todd Lipcon has posted comments on this change. Change subject: tools: add a tool to summarize data size on a tablet or tablets ......................................................................
Patch Set 2: (14 comments) http://gerrit.cloudera.org:8080/#/c/5727/2/src/kudu/tools/kudu-tool-test.cc File src/kudu/tools/kudu-tool-test.cc: Line 387: const vector<string> kLocalReplicaModeRegexes = { > Update this? Done Line 1119: "KuduTableTestId | test-tablet | 0 | c10 (key) | \\S+\n", > Why are these column IDs c10, c11, c12 and not c1, c2, c3? AFAICT the schem in debug builds the column ids start at 10. I'm surprised this didn't fail in a release build, though... my guess is that the '|' in these patterns is getting interpreted as 'OR', so the assertions are actually whack. I futzed with regex escaping for a while and eventually gave up and went with a new approach. Line 1143: " KuduTableTestId | * | * | * | \\S+\n"}) { > Nit: fix indentation? Looks like some rows are written differently than oth my editor doesn't like multiline strings http://gerrit.cloudera.org:8080/#/c/5727/2/src/kudu/tools/tool_action_common.h File src/kudu/tools/tool_action_common.h: Line 65: extern const char* const kTabletIdGlobArg; > You could define these just once in tool_action_local_replica.cc, since the Done http://gerrit.cloudera.org:8080/#/c/5727/1/src/kudu/tools/tool_action_local_replica.cc File src/kudu/tools/tool_action_local_replica.cc: PS1, Line 375: const vector<BlockId>& blocks, : StringPiece block_type, > Would be nice to relegate this to "--verbose" or some such. did check against VLOG(1) Line 381: Substitute("could not open block $0", b.ToString())); > As we discussed, it would also be nice to be able to sum up redo or undo or done Line 411: > This one too. Done http://gerrit.cloudera.org:8080/#/c/5727/2/src/kudu/tools/tool_action_local_replica.cc File src/kudu/tools/tool_action_local_replica.cc: Line 385: *running_sum += size; > Nit: let's only update the OUT param on success. Done Line 386: if (VLOG_IS_ON(1)) { > Do you want to piggy-back on the CLI's existing --verbose flag for this? I don't see such a flag: todd@va1022:~/kudu$ ./build/latest/bin/kudu --verbose ERROR: unknown command line flag 'verbose' Line 387: cout << block_type << " block " << b.ToString() << ": " > Nit: Substitute() so this is more readable? Done Line 447: if (!MatchPattern(tablet_id, tablet_id_pattern)) continue; > Should probably fully document the pattern matching semantics in the tool's given that this is on tablet IDs I think really only '*' is particularly useful (so you can just type a prefix and use '*' to complete it). Tweaked the help a bit though. Line 471: (col_idx != Schema::kColumnNotFound) ? > Is that possible? For a dropped column? Would be nice to get some test cove yea for dropped. I think in this context it's a bit tricky to engineer the dropped column (no good utility code to do so). Line 969: .Description("Summarize the data size/space usage of the given local replica(s).") > It would also be nice to slice the data vertically. For example, I want to It already does show the sum of redo/undo/bloom/pk/each-col across each table. Summing across all tables isn't done but how about leaving that for a future enhancement? Line 982: .AddAction(std::move(data_size)) > Nit: resort alphabetically. Done -- To view, visit http://gerrit.cloudera.org:8080/5727 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ibf0237f9d01f4ec332d1df545d2c2f51f64ce012 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Jean-Daniel Cryans <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Reviewer: Will Berkeley <[email protected]> Gerrit-HasComments: Yes
