Adar Dembo has posted comments on this change. Change subject: tool: port log-dump ......................................................................
Patch Set 3: (3 comments) http://gerrit.cloudera.org:8080/#/c/4167/3/src/kudu/tools/kudu-tool-test.cc File src/kudu/tools/kudu-tool-test.cc: 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". http://gerrit.cloudera.org:8080/#/c/4167/3/src/kudu/tools/tool_action_tablet.cc File src/kudu/tools/tool_action_tablet.cc: 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 http://gerrit.cloudera.org:8080/4167 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I14f048169c0b211e3c72fcd255c76dd59cbb05c9 Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo <a...@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
