Adar Dembo has posted comments on this change.

Change subject: tool: port log-dump

Patch Set 3:

File src/kudu/tools/

PS3, Line 92: out
> Some basic paranoia Qn unrelated to your change: The string object itself s
The string _data_ itself is heap-allocated; std::string is just a small 
container object.

Line 369:   const Schema kSchemaWithIds(SchemaBuilder(kSchema).Build());
> I am curious to know the purpose of SchemaBuilder here ? Can't we directly 
Apparently not. I copied this from log-test-base.h because when I tried to use 
kSchema as-is in Log::Open(), I got a log that could not be read later on. I 
guess you need to rebuild the schema with column IDs like this. I agree that 
it's unintuitive; I was just trying to "get it to work".
File src/kudu/tools/

PS3, Line 265: AddOptionalParameter
> I have been wondering about this while testing today: isn't fs_wal_dir supp
fs_wal_dir and fs_data_dirs are "optional" in the sense that they're gflags and 
thus can be omitted from the command line and the tool still run. However, 
FsManager::Open will fail if their values aren't provided.

This is confusing because all other gflags are optional. For a while I thought 
maybe we should add required (positional) parameters for these two, then map 
their values to gflags values when parsing. But then the number of positional 
parameters is quite high, and that's annoying to deal with (i.e. it's easy to 
get them out of order on the command line).

So in a nutshell, I'm not really sure how we should handle them. We could make 
it explicit that some keyval args (i.e. --fs_wal_dir) are required, but that's 
not Unix-y.

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I14f048169c0b211e3c72fcd255c76dd59cbb05c9
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <>
Gerrit-Reviewer: Adar Dembo <>
Gerrit-Reviewer: Dinesh Bhat <>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <>
Gerrit-HasComments: Yes

Reply via email to