[BEAM-10670] Make Read execute as a splittable DoFn by default for the Java DirectRunner. by lukecwik · Pull Request #12519 · apache/beam

lukecwik · 2020-08-10T20:33:14Z

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.
Update CHANGES.md with noteworthy changes.
If this contribution is large, please file an Apache Individual Contributor License Agreement.

See the Contributor Guide for more tips on how to make review process smoother.

Post-Commit Tests Status (on master branch)

Lang	Dataflow	Samza	Twister2
Go	---	---	---
Java
Python		---	---
XLang	---	---	---

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.

GitHub Actions Tests Status (on master branch)

See CI.md for more information about GitHub Actions CI.

lukecwik · 2020-08-10T20:34:57Z

This is effectively a duplicate of AvroIOTest$SimpleTests.testReadDisplayData and doesn't need to a be a validates runner test since it doesn't run a pipeline.

lukecwik · 2020-08-10T20:37:16Z

(here and below) This was previously throwing the top level exception but DirectRunner wraps ParDo exceptions with PipelineExecutionException but BoundedSource/UnboundedSource used a different code path and hence a different exception handling path.

lukecwik · 2020-08-10T20:37:37Z

Effectively a duplicate of testReadDisplayData just above.

lukecwik · 2020-08-10T21:00:54Z

R: @boyuanzz

Discussion in https://lists.apache.org/thread.html/r1ba6fe6ac2bd2b28aa7ef31f0d87ad716fc878f2515085fdbc275333%40%3Cdev.beam.apache.org%3E but I would like to get this reviewed and be able to merge it once that discussion subsides.

lukecwik · 2020-08-10T22:34:59Z

Run Java PreCommit

lukecwik · 2020-08-11T18:04:56Z

Run JavaPortabilityApiJava11 PreCommit

boyuanzz · 2020-08-11T21:04:09Z

Suggested change
* containanyprimitivereads.
* containanyprimitivereadswhenuse_deprecated_readisnotspecified.

boyuanzz · 2020-08-11T21:09:31Z

Is it possible to make beam_fn_api_use_deprecated_read and use_deprecated_read into one use_deprecated_read since they seem to the same.

I wanted to use one that wasn't coupled to beam_fn_api so that users of non portable pipelines wouldn't be confused. I also don't want to impact existing users until we start removing the experiment so I needed to support both.

boyuanzz · 2020-08-11T21:24:58Z

Can we add one block to test using use_deprecated_read ?

boyuanzz · 2020-08-11T21:27:22Z

Is it because we have checkDone now?

I added this check back and altered how we were passing forward the self-checkpoing signal from the UnboundedSource through the SDF wrapper instead.
The check is valid.

lukecwik · 2020-08-11T22:32:12Z

Thanks for taking a look, I'm going to break this up into a few PRs to separate it from what was needed for the DirectRunner.

lukecwik · 2020-08-12T17:53:20Z

I split off changes that were not as releated to migrating DirectRunner over to use SDF in PRs #12547 and #12548
I have one more related to HCatalog but it is only a workaround and not a real fix.

…e Java DirectRunner.

lukecwik · 2020-08-14T21:00:50Z

Run Python2_PVR_Flink PreCommit

boyuanzz · 2020-08-14T21:13:41Z

Run Python2_PVR_Flink PreCommit

I think something for :sdks:go:buildLinuxAmd64 is broken. I also got similar errors from my PR which is unexpected.

boyuanzz

Thanks~

lukecwik · 2020-08-15T00:10:44Z

Run Java PreCommit

lukecwik · 2020-08-15T00:10:49Z

Run Portable_Python PreCommit

lukecwik · 2020-08-15T00:10:55Z

Run Python2_PVR_Flink PreCommit

lukecwik · 2020-08-15T01:28:36Z

Run Java PreCommit

lukecwik · 2020-08-15T02:16:03Z

Run Java PreCommit

lukecwik · 2020-08-15T04:34:46Z

Run Java PreCommit

lukecwik · 2020-08-15T09:42:00Z

Run Java PreCommit

lukecwik · 2020-08-15T18:46:42Z

Run Java PreCommit

tvalentyn · 2020-08-21T00:49:48Z

It appears as though this change has increased the volume of Java Precommit warning output ~4x.

https://ci-beam.apache.org/job/beam_PreCommit_Java_Cron/3124 lists this as the single change.

Console output for previous build is 3MB: https://ci-beam.apache.org/job/beam_PreCommit_Java_Cron/3123/console
Console output for this build is 13MB: https://ci-beam.apache.org/job/beam_PreCommit_Java_Cron/3124/consoleFull

and has lots of entries like:

17:13:30 WARNING: Coder of type class org.apache.beam.sdk.coders.KvCoder has a #structuralValue method which does not return true when the encoding of the elements is equal. Element KV{[67, -53, -64, -6, 101, -59, 69, 36, 85, -98, 87, -102, 85, -46, 18, -108, 46, -45, -68, 64, -13, 60, -114, -76, 11, 114, 96, -7, 67, -37, -114, -85, 76, 57, -29, 99, -25, -69, 56, -122, -7, 19, -17, -93, 32, 74, -22, 95, 68, 13, -113, 121, 95, 9, 31, 99, -112, 27, 79, -29, -48, -65, -100, -4, -75, -81, 56, 13, -112, 20, 116, 89, 53, 28, 118, -2, -59, 93, -17, 1, 0, 56, 91, 9, -71, 6, 127, 65, 5, 9, -57, -59, 119, 31, -92, 34, -96, -118, -49, -60, 4, 36, 90, 97, 7, 7, 59, -90, -87, 107, -124, 31, 50, 101, -127, -28, -31, 0, 77, -82, 91, -98, 33, -16, 15, 23, 21, 120], KV{[0, 12), [0, 1)}}

cc: @abhiy13 who brought this to my attention.

lukecwik · 2020-08-21T17:03:03Z

It appears as though this change has increased the volume of Java Precommit warning output ~4x.
https://ci-beam.apache.org/job/beam_PreCommit_Java_Cron/3124 lists this as the single change.
Console output for previous build is 3MB: https://ci-beam.apache.org/job/beam_PreCommit_Java_Cron/3123/console
Console output for this build is 13MB: https://ci-beam.apache.org/job/beam_PreCommit_Java_Cron/3124/consoleFull
and has lots of entries like:
17:13:30 WARNING: Coder of type class org.apache.beam.sdk.coders.KvCoder has a #structuralValue method which does not return true when the encoding of the elements is equal. Element KV{[67, -53, -64, -6, 101, -59, 69, 36, 85, -98, 87, -102, 85, -46, 18, -108, 46, -45, -68, 64, -13, 60, -114, -76, 11, 114, 96, -7, 67, -37, -114, -85, 76, 57, -29, 99, -25, -69, 56, -122, -7, 19, -17, -93, 32, 74, -22, 95, 68, 13, -113, 121, 95, 9, 31, 99, -112, 27, 79, -29, -48, -65, -100, -4, -75, -81, 56, 13, -112, 20, 116, 89, 53, 28, 118, -2, -59, 93, -17, 1, 0, 56, 91, 9, -71, 6, 127, 65, 5, 9, -57, -59, 119, 31, -92, 34, -96, -118, -49, -60, 4, 36, 90, 97, 7, 7, 59, -90, -87, 107, -124, 31, 50, 101, -127, -28, -31, 0, 77, -82, 91, -98, 33, -16, 15, 23, 21, 120], KV{[0, 12), [0, 1)}}
cc: @abhiy13 who brought this to my attention.

Filed https://issues.apache.org/jira/browse/BEAM-10788

iemejia · 2020-08-25T21:27:27Z

From looking at the Nexmark tests it seems this change improved the performance of the DirectRunner in almost all queries considerably, this is interesting! I expected worse because of the extra translation but I did not see any change that made sense to me, but well I suppose this is more the consequences of previous PRs, anyway just letting this as a note.

probot-autolabelerBot added core direct java runners labels Aug 10, 2020

lukecwik commented Aug 10, 2020
View reviewed changes

probot-autolabelerBot added euphoria extensions labels Aug 11, 2020

boyuanzz reviewed Aug 11, 2020
View reviewed changes

lukecwik changed the title ~~[BEAM-10670] Make Read execute as a splittable DoFn by default for the Java DirectRunner.~~[WIP][BEAM-10670] Make Read execute as a splittable DoFn by default for the Java DirectRunner.Aug 11, 2020

lukecwik mentioned this pull request Aug 12, 2020
[BEAM-10694] Work around serialization issue with ReaderContext by memoizing the serialized form and propagating it forward. #12556
Merged
4 tasks

lukecwik force-pushed the beam10670 branch from 90c249a to b3d52f1Compare August 14, 2020 20:52

probot-autolabelerBot added gcp io labels Aug 14, 2020

[BEAM-10670] Make Read execute as a splittable DoFn by default for th…
2e7c4ca
…e Java DirectRunner.

lukecwik force-pushed the beam10670 branch from b3d52f1 to 2e7c4caCompare August 14, 2020 20:55

lukecwik changed the title ~~[WIP][BEAM-10670] Make Read execute as a splittable DoFn by default for the Java DirectRunner.~~[BEAM-10670] Make Read execute as a splittable DoFn by default for the Java DirectRunner.Aug 14, 2020

boyuanzz approved these changes Aug 14, 2020
View reviewed changes

lukecwik merged commit c720f2a into apache:masterAug 15, 2020

lukecwik mentioned this pull request Aug 21, 2020
[BEAM-10788] Also disable immutability checking for transforms within Read. #12660
Merged
4 tasks

lukecwik mentioned this pull request Oct 5, 2020
[BEAM-10670] Make Read use SDF by default. Override in runners. #13006
Merged
4 tasks

	* containanyprimitivereads.
	* containanyprimitivereadswhenuse_deprecated_readisnotspecified.

Uh oh!

Conversation

lukecwik commented Aug 10, 2020

Post-Commit Tests Status (on master branch)

Pre-Commit Tests Status (on master branch)

GitHub Actions Tests Status (on master branch)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lukecwik commented Aug 10, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lukecwik commented Aug 10, 2020

Uh oh!

lukecwik commented Aug 11, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lukecwik commented Aug 11, 2020

Uh oh!

lukecwik commented Aug 12, 2020

Uh oh!

lukecwik commented Aug 14, 2020

Uh oh!

boyuanzz commented Aug 14, 2020

Uh oh!

boyuanzz left a comment

Choose a reason for hiding this comment

Uh oh!

lukecwik commented Aug 15, 2020

Uh oh!

lukecwik commented Aug 15, 2020

Uh oh!

lukecwik commented Aug 15, 2020

Uh oh!

lukecwik commented Aug 15, 2020

Uh oh!

lukecwik commented Aug 15, 2020

Uh oh!

lukecwik commented Aug 15, 2020

Uh oh!

lukecwik commented Aug 15, 2020

Uh oh!

lukecwik commented Aug 15, 2020

Uh oh!

tvalentyn commented Aug 21, 2020

Uh oh!

lukecwik commented Aug 21, 2020

Uh oh!

iemejia commented Aug 25, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

lukecwik commented Aug 10, 2020•
edited
Loading