You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Todd Lipcon (Code Review)" <ge...@cloudera.org> on 2017/06/02 00:41:49 UTC
[kudu-CR] tool: add a 'pbc edit' command
Hello Mike Percy,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/7048
to review the following change.
Change subject: tool: add a 'pbc edit' command
......................................................................
tool: add a 'pbc edit' command
This adds a new command 'kudu pbc edit' which can be used to edit any
PBC file. It dumps the file in JSON format, then opens the user's
$EDITOR, and then re-encodes it back to PBC.
Change-Id: Ie9434935469df8978a4f3fb3719f0245499aece5
---
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_pbc.cc
M src/kudu/util/pb_util-test.cc
M src/kudu/util/pb_util.cc
M src/kudu/util/pb_util.h
5 files changed, 340 insertions(+), 50 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/48/7048/1
--
To view, visit http://gerrit.cloudera.org:8080/7048
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ie9434935469df8978a4f3fb3719f0245499aece5
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
[kudu-CR] tool: add a 'pbc edit' command
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: tool: add a 'pbc edit' command
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7048/1/src/kudu/tools/tool_action_pbc.cc
File src/kudu/tools/tool_action_pbc.cc:
Line 173: for (const string& l : lines) {
> Protobuf is just writing a single JSON "object" per serialized message.
Yeah I'm fine with deferring this improvement for later.
--
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 <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] tool: add a 'pbc edit' command
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has submitted this change and it was merged.
Change subject: tool: add a 'pbc edit' command
......................................................................
tool: add a 'pbc edit' command
This adds a new command 'kudu pbc edit' which can be used to edit any
PBC file. It dumps the file in JSON format, then opens the user's
$EDITOR, and then re-encodes it back to PBC.
Change-Id: Ie9434935469df8978a4f3fb3719f0245499aece5
Reviewed-on: http://gerrit.cloudera.org:8080/7048
Reviewed-by: Adar Dembo <ad...@cloudera.com>
Tested-by: Kudu Jenkins
Reviewed-by: Mike Percy <mp...@apache.org>
---
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_pbc.cc
M src/kudu/util/pb_util-test.cc
M src/kudu/util/pb_util.cc
M src/kudu/util/pb_util.h
5 files changed, 342 insertions(+), 50 deletions(-)
Approvals:
Mike Percy: Looks good to me, approved
Adar Dembo: Looks good to me, but someone else must approve
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/7048
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: Ie9434935469df8978a4f3fb3719f0245499aece5
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] tool: add a 'pbc edit' command
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
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 <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] tool: add a 'pbc edit' command
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
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 <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] tool: add a 'pbc edit' command
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.
Change subject: tool: add a 'pbc edit' command
......................................................................
Patch Set 2: Code-Review+2
(2 comments)
http://gerrit.cloudera.org:8080/#/c/7048/1/src/kudu/tools/tool_action_pbc.cc
File src/kudu/tools/tool_action_pbc.cc:
PS1, Line 142: tmp_template),
> mktemp unfortunately requires the last 6 characters to be XXXXXX. Added the
ah, we would have to use mkstemps(). ok forget it
Line 173: for (const string& l : lines) {
> Yeah I'm fine with deferring this improvement for later.
Yeah, that's fine.
--
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: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] tool: add a 'pbc edit' command
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: tool: add a 'pbc edit' command
......................................................................
Patch Set 1:
(1 comment)
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. I guess the iteration order is build-dependent (or probably libc++ vs libstdcxx-dependent). Will fix in next rev.
--
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 <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] tool: add a 'pbc edit' command
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: tool: add a 'pbc edit' command
......................................................................
Patch Set 2: Code-Review+1
Deferring to Mike.
--
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: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] tool: add a 'pbc edit' command
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: tool: add a 'pbc edit' command
......................................................................
Patch Set 1:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/7048/1/src/kudu/tools/tool_action_pbc.cc
File src/kudu/tools/tool_action_pbc.cc:
PS1, Line 142: "pbc-edit.XXXXXX"
> If we name it "pbc-edit.XXXXXX.json" then vim / emacs will give us syntax h
We should also include the standard Kudu tmpdir infix so that if you leave one behind in a data directory it'll get cleaned up.
Line 173: for (const string& l : lines) {
> one per line can be hard to edit for larger messages. Would it be difficult
Agreed. How does protobuf write out the JSON-encoded messages? I presume it's a top-level JSON array of messages, so perhaps we could parse with rapidjson, iterate over the top-level array, serialize each record _back_ to JSON, then hand that off to JsonStringToMessage()?
It's excessive, but it should allow for more flexibility in the editing process.
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 you close it.
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 the protobuf JSON parser thing to sort the keys or something?
--
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 <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] tool: add a 'pbc edit' command
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7048
to look at the new patch set (#2).
Change subject: tool: add a 'pbc edit' command
......................................................................
tool: add a 'pbc edit' command
This adds a new command 'kudu pbc edit' which can be used to edit any
PBC file. It dumps the file in JSON format, then opens the user's
$EDITOR, and then re-encodes it back to PBC.
Change-Id: Ie9434935469df8978a4f3fb3719f0245499aece5
---
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_pbc.cc
M src/kudu/util/pb_util-test.cc
M src/kudu/util/pb_util.cc
M src/kudu/util/pb_util.h
5 files changed, 342 insertions(+), 50 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/48/7048/2
--
To view, visit http://gerrit.cloudera.org:8080/7048
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie9434935469df8978a4f3fb3719f0245499aece5
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>