Mike Percy has posted comments on this change. Change subject: tool: add a 'pbc edit' command ......................................................................
Patch Set 1: (5 comments) http://gerrit.cloudera.org:8080/#/c/7048/1/src/kudu/tools/tool_action_pbc.cc File src/kudu/tools/tool_action_pbc.cc: Line 60: const char* const kPathArg = "pbc-path"; Not sure we get any value from changing this, I'm guessing you originally needed to because you were doing something like a "create". PS1, Line 142: "pbc-edit.XXXXXX" If we name it "pbc-edit.XXXXXX.json" then vim / emacs will give us syntax highlighting PS1, Line 159: RETURN_NOT_OK(RunEditor(tmp_json_path)); This is cool, but I wonder if we shouldn't just do "create" instead of "edit", or provide both. However, I understand the motivation behind doing it this way; if the file already exists then we can easily get the schema without the user having to pass it in, etc... so maybe this is the best option after all. Line 173: for (const string& l : lines) { one per line can be hard to edit for larger messages. Would it be difficult to represent it as an array of hashes and pretty-print it? So in the editor we end up with something like this, which should be easier to edit and not screw up: [ { "foo": "bar", "baz": "blam" }, { "foo": "bar2", "baz": "blam2" } ] PS1, Line 187: env->NowMicros() Hmm, this is specified in env.h as something like tick time but I think it's implemented as wall time... so I guess it's fine. I would have thought to use GetCurrentTimeMicros(). We should probably get rid of one, although that's not really relevant to this patch. -- To view, visit http://gerrit.cloudera.org:8080/7048 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ie9434935469df8978a4f3fb3719f0245499aece5 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
