[BEAM-2431] Add PipelineOptionsTranslation #3719
Conversation
This converts a PipelineOptions instance to and from a Protobuf Struct.
|
R: @kennknowles |
tgroh
changed the title
There was a problem hiding this comment.
Fine as-is, but some comments.
@lukecwik might have suggestions.
|
|
||
| @Test | ||
| public void testToFromProto() throws Exception { | ||
| PipelineOptionsTranslation.fromProto(PipelineOptionsTranslation.toProto(options)); |
There was a problem hiding this comment.
Is there a way to check that you got the right thing back?
There was a problem hiding this comment.
You should be able to convert it to JSON and perform a JSON equivalence test.
Proto3 struct may do the equivalence check as well.
There was a problem hiding this comment.
I need to add a call to getOptionsId for this to pass. Otherwise, done.
| PipelineOptions withParsedSettings = | ||
| PipelineOptionsFactory.fromArgs("--jobName=my_job --appName=my_app").create(); | ||
|
|
||
| return Arrays.asList( |
There was a problem hiding this comment.
nit: I like ImmutableList.of(...) more than Arrays.asList(...) just because you aren't actually converting an array and that's a bizarre Java-ism.
|
|
||
| @Test | ||
| public void testToFromProto() throws Exception { | ||
| PipelineOptionsTranslation.fromProto(PipelineOptionsTranslation.toProto(options)); |
There was a problem hiding this comment.
You should be able to convert it to JSON and perform a JSON equivalence test.
Proto3 struct may do the equivalence check as well.
| public static Struct toProto(PipelineOptions options) { | ||
| Struct.Builder builder = Struct.newBuilder(); | ||
| try { | ||
| JsonFormat.parser().merge(MAPPER.writeValueAsString(options), builder); |
There was a problem hiding this comment.
It is a bit of a bummer to write to a JSON string only to parse it immediately. I don't have a strong opinion, but you might want to add a comment so the next person coming here is not shocked. And if they know a better way they can fix it. (but this is not high bandwidth so I don't think you should spend time trying to improve it unless there's something obvious)
There was a problem hiding this comment.
Yeah. Commented a touch, referencing this PR.
Follow this checklist to help us incorporate your contribution quickly and easily:
[BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replaceBEAM-XXXwith the appropriate JIRA issue.mvn clean verifyto make sure basic checks pass. A more thorough check will be performed on your pull request automatically.This converts a PipelineOptions instance to and from a Protobuf Struct.
Needs to be performed to send an entire job specification through the Job API.