Todd Lipcon has posted comments on this change. Change subject: tool: add a 'pbc edit' command ......................................................................
Patch Set 1: (9 comments) http://gerrit.cloudera.org:8080/#/c/7048/1/src/kudu/tools/kudu-tool-test.cc File src/kudu/tools/kudu-tool-test.cc: Line 740: ": missing field uuid", stderr); > the TSAN build failed because it called out a different missing field here. Done http://gerrit.cloudera.org:8080/#/c/7048/1/src/kudu/tools/tool_action_pbc.cc File src/kudu/tools/tool_action_pbc.cc: Line 41: using std::endl; > warning: using decl 'endl' is unused [misc-unused-using-decls] Done Line 60: const char* const kPathArg = "pbc-path"; > Not sure we get any value from changing this, I'm guessing you originally n You're exactly right. Done PS1, Line 142: "pbc-edit.XXXXXX" > We should also include the standard Kudu tmpdir infix so that if you leave mktemp unfortunately requires the last 6 characters to be XXXXXX. Added the tmp infinx. 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 "edi yea, I originally started with 'export' and 'import', but ran into the problem of knowing the schema. For export/import to work, we'd have to JSONify the MessageDescriptor into the 'exported' file, which, while possible, is also kind of a pain. Sounds like you don't feel strongly, but if you do, lmk and I'll give that a try. Line 173: for (const string& l : lines) { > Agreed. How does protobuf write out the JSON-encoded messages? I presume it Protobuf is just writing a single JSON "object" per serialized message. I could certainly change it to output a '[' at the beginning and a ',' between each message so that it ends up writing an array, and pretty-print. As you identified, the issue is on re-parsing back this format. Using RapidJSON is doable, or I could potentially create a Descriptor on the fly which just contains a 'repeated FooProto' and try to serialize that. Or even duplicate a small amount of the code from protobuf/util/json_util.cc to do this special-purpose task. All of these are bit of work, though. Mind if we commit this as is for now and we can do the changes as a follow-up, since this is an interactive admin tool and therefore wouldn't have compatibility issues to improve later? PS1, Line 187: env->NowMicros() > Hmm, this is specified in env.h as something like tick time but I think it' Done Line 194: WARN_NOT_OK(env->SyncDir(dir), "couldn't sync directory"); > If you're going to sync the directory you should also sync pb_writer before Done http://gerrit.cloudera.org:8080/#/c/7048/1/src/kudu/util/pb_util-test.cc File src/kudu/util/pb_util-test.cc: Line 557: const char* kExpectedOutputJson = > Is this expected output stable? Can we ensure that it's stable by asking th I believe it's stable based on the order in the descriptor. There is no option in the output to sort it. -- 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
