[BEAM-3344] Changes for serialize/deserialize PipelineOptions via jackson in DirectRunner #8302

sudhan499 · 2019-04-13T21:06:43Z

Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:

Choose reviewer(s) and mention them in a comment (R: @username).
Format the pull request title like [BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replace BEAM-XXX with the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.
If this contribution is large, please file an Apache Individual Contributor License Agreement.

Post-Commit Tests Status (on master branch)

Lang	Apex	Dataflow	Flink	Gearpump	Samza	Spark
Go	---	---	---	---	---	---
Java
Python	---			---	---	---

Pre-Commit Tests Status (on master branch)

---	Java	Python	Go	Website
Non-portable
Portable	---		---	---

See .test-infra/jenkins/README for trigger phrase, status and link of all Jenkins jobs.

…kson in DirectRunner

…ctRunner

sudhan499 · 2019-04-13T22:14:18Z

R: @lukecwik

sudhan499 · 2019-04-15T02:15:37Z

retest this please

lukecwik · 2019-04-18T19:11:39Z

Run Portable_Python PreCommit

lukecwik · 2019-04-18T19:11:48Z

Run Java PreCommit

lukecwik · 2019-04-18T19:20:50Z

runners/direct-java/src/main/java/org/apache/beam/runners/direct/DirectRunner.java


  /** Construct a {@link DirectRunner} from the provided options. */
  public static DirectRunner fromOptions(PipelineOptions options) {
+    options = MAPPER.convertValue(MAPPER.valueToTree(options), DirectOptions.class);


convertValue / valueToTree may go through specialized paths which partially convert value types and allow for things like static references to stick around. Instead try using ObjectMapper#readValue and ObjectMapper#writeValues

Can you also add a unit test?

@lukecwik Thanks for the suggestions.
Added changes as suggested and also added unit test for the same. Please review my changes.

…son in DirectRunner

sudhan499 · 2019-04-23T16:39:21Z

retest this please

sudhan499 · 2019-04-23T17:39:43Z

@lukecwik Could you please review my pull request.

aaltay · 2019-04-25T00:34:25Z

Run Java PreCommit

aaltay · 2019-04-25T00:34:45Z

Run Portable_Python PreCommit

runners/direct-java/src/main/java/org/apache/beam/runners/direct/DirectRunner.java

runners/direct-java/src/test/java/org/apache/beam/runners/direct/DirectRunnerTest.java

lukecwik · 2019-04-26T20:56:55Z

All minor suggestions, please take a look and accept changes that you agree with.

lukecwik · 2019-04-26T20:57:13Z

Run Java PreCommit

lukecwik · 2019-04-26T20:57:18Z

Run Portable_Python PreCommit

…ct/DirectRunner.java Co-Authored-By: sudhan499 <sudhan499@gmail.com>

…ct/DirectRunnerTest.java Co-Authored-By: sudhan499 <sudhan499@gmail.com>

sudhan499 · 2019-04-27T05:33:53Z

retest this please

sudhan499 · 2019-04-27T11:30:20Z

@lukecwik Thanks for the suggestions and time. I agree with all the changes you suggested and accepted them.

lukecwik · 2019-04-29T15:33:40Z

Run Java PreCommit

…s invoked to allow for the correct binding of RuntimeValueProvider to occur.

lukecwik

I was looking into the test failure for the PR related to DatastoreV1Test and found that doing the serialization/deserialization at construction time instead of at run() causes the ValueProviders to become accessible which breaks all the tests that rely on checking ValueProviders not being set. I opened this PR against your repo with a fix. If you think this still addresses BEAM-3344, please merge my PR into your repo so that this PR gets updated and then we can make sure the tests are passing.

Migrate PipelineOptions serialization/deserialization till when run is invoked to allow for the correct binding of RuntimeValueProvider to occur.

sudhan499 · 2019-04-30T09:45:15Z

@lukecwik Yes, your changes will still address BEAM-3344 issue and also it fixed the failing tests of DataStoreV1Test . Thanks for helping me in this.

lukecwik · 2019-04-30T20:37:36Z

Thanks, everything looks good and the tests passed.

sudhan499 added 2 commits April 14, 2019 02:30

[BEAM-3344] CHanges for serialize/deserialize PipelineOptions via jac…

657c14f

…kson in DirectRunner

[BEAM-3344] Serialize/deserialize PipelineOptions via Jackson in Dire…

8a82d73

…ctRunner

lukecwik requested changes Apr 18, 2019

View reviewed changes

lukecwik changed the title ~~[BEAM-3344] Changes for serialize/deserialize PipelineOptions via jac…~~ [BEAM-3344] Changes for serialize/deserialize PipelineOptions via jackson in DirectRunner Apr 18, 2019

sudhan499 added 3 commits April 20, 2019 23:44

BEAM-3344] Changes for serialize/deserialize PipelineOptions via jack…

f40abac

…son in DirectRunner

[BEAM-3344] Minor fix Marking interface as public

6991291

[BEAM-3344] Fixing Checkstyle issue.

5f7708b

lukecwik requested changes Apr 26, 2019

View reviewed changes

lukecwik and others added 11 commits April 27, 2019 10:58

Update runners/direct-java/src/main/java/org/apache/beam/runners/dire…

f816147

…ct/DirectRunner.java Co-Authored-By: sudhan499 <sudhan499@gmail.com>

Update runners/direct-java/src/main/java/org/apache/beam/runners/dire…

83781e0

…ct/DirectRunner.java Co-Authored-By: sudhan499 <sudhan499@gmail.com>

Update runners/direct-java/src/main/java/org/apache/beam/runners/dire…

57f0d75

…ct/DirectRunner.java Co-Authored-By: sudhan499 <sudhan499@gmail.com>

Update runners/direct-java/src/main/java/org/apache/beam/runners/dire…

d890997

…ct/DirectRunner.java Co-Authored-By: sudhan499 <sudhan499@gmail.com>

Update runners/direct-java/src/test/java/org/apache/beam/runners/dire…

8a0e019

…ct/DirectRunnerTest.java Co-Authored-By: sudhan499 <sudhan499@gmail.com>

Update runners/direct-java/src/test/java/org/apache/beam/runners/dire…

2758a93

…ct/DirectRunnerTest.java Co-Authored-By: sudhan499 <sudhan499@gmail.com>

Update runners/direct-java/src/test/java/org/apache/beam/runners/dire…

1c731d4

…ct/DirectRunnerTest.java Co-Authored-By: sudhan499 <sudhan499@gmail.com>

Update runners/direct-java/src/test/java/org/apache/beam/runners/dire…

beafb90

…ct/DirectRunnerTest.java Co-Authored-By: sudhan499 <sudhan499@gmail.com>

Update runners/direct-java/src/test/java/org/apache/beam/runners/dire…

81aae84

…ct/DirectRunnerTest.java Co-Authored-By: sudhan499 <sudhan499@gmail.com>

Update runners/direct-java/src/test/java/org/apache/beam/runners/dire…

13d70cc

…ct/DirectRunnerTest.java Co-Authored-By: sudhan499 <sudhan499@gmail.com>

Update runners/direct-java/src/test/java/org/apache/beam/runners/dire…

e33401f

…ct/DirectRunnerTest.java Co-Authored-By: sudhan499 <sudhan499@gmail.com>

lukecwik and others added 3 commits April 27, 2019 11:01

Update runners/direct-java/src/test/java/org/apache/beam/runners/dire…

942003b

…ct/DirectRunnerTest.java Co-Authored-By: sudhan499 <sudhan499@gmail.com>

Update runners/direct-java/src/test/java/org/apache/beam/runners/dire…

eb2db2f

…ct/DirectRunnerTest.java Co-Authored-By: sudhan499 <sudhan499@gmail.com>

Update runners/direct-java/src/test/java/org/apache/beam/runners/dire…

23074e8

…ct/DirectRunnerTest.java Co-Authored-By: sudhan499 <sudhan499@gmail.com>

[BEAM-3344] Applying spotless java changes

5c2a501

lukecwik approved these changes Apr 29, 2019

View reviewed changes

Migrate PipelineOptions serialization/deserialization till when run i…

57d3f51

…s invoked to allow for the correct binding of RuntimeValueProvider to occur.

lukecwik requested changes Apr 29, 2019

View reviewed changes

Merge pull request #1 from lukecwik/pr8302

b9570bb

Migrate PipelineOptions serialization/deserialization till when run is invoked to allow for the correct binding of RuntimeValueProvider to occur.

lukecwik merged commit 3839281 into apache:master Apr 30, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BEAM-3344] Changes for serialize/deserialize PipelineOptions via jackson in DirectRunner #8302

[BEAM-3344] Changes for serialize/deserialize PipelineOptions via jackson in DirectRunner #8302

sudhan499 commented Apr 13, 2019 •

edited by lukecwik

sudhan499 commented Apr 13, 2019

sudhan499 commented Apr 15, 2019

lukecwik commented Apr 18, 2019

lukecwik commented Apr 18, 2019

lukecwik Apr 18, 2019

sudhan499 Apr 20, 2019

sudhan499 commented Apr 23, 2019

sudhan499 commented Apr 23, 2019

aaltay commented Apr 25, 2019

aaltay commented Apr 25, 2019

lukecwik commented Apr 26, 2019

lukecwik commented Apr 26, 2019

lukecwik commented Apr 26, 2019

sudhan499 commented Apr 27, 2019

sudhan499 commented Apr 27, 2019

lukecwik commented Apr 29, 2019

lukecwik left a comment

sudhan499 commented Apr 30, 2019

lukecwik commented Apr 30, 2019

[BEAM-3344] Changes for serialize/deserialize PipelineOptions via jackson in DirectRunner #8302

[BEAM-3344] Changes for serialize/deserialize PipelineOptions via jackson in DirectRunner #8302

Conversation

sudhan499 commented Apr 13, 2019 • edited by lukecwik

Post-Commit Tests Status (on master branch)

Pre-Commit Tests Status (on master branch)

sudhan499 commented Apr 13, 2019

sudhan499 commented Apr 15, 2019

lukecwik commented Apr 18, 2019

lukecwik commented Apr 18, 2019

lukecwik Apr 18, 2019

Choose a reason for hiding this comment

sudhan499 Apr 20, 2019

Choose a reason for hiding this comment

sudhan499 commented Apr 23, 2019

sudhan499 commented Apr 23, 2019

aaltay commented Apr 25, 2019

aaltay commented Apr 25, 2019

lukecwik commented Apr 26, 2019

lukecwik commented Apr 26, 2019

lukecwik commented Apr 26, 2019

sudhan499 commented Apr 27, 2019

sudhan499 commented Apr 27, 2019

lukecwik commented Apr 29, 2019

lukecwik left a comment

Choose a reason for hiding this comment

sudhan499 commented Apr 30, 2019

lukecwik commented Apr 30, 2019

sudhan499 commented Apr 13, 2019 •

edited by lukecwik