[BEAM-8932][Cleanup] Move external PubsubIO hooks outside of PubsubIO. by dpcollins-google · Pull Request #10476 · apache/beam

dpcollins-google · 2019-12-28T06:08:59Z

Also fix these so that they typecheck correctly without relying on java runtime duck typing.

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

[x ] Choose reviewer(s) and mention them in a comment (R: @username).
[ x] 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.
[x ] 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	SDK	Apex	Dataflow	Gearpump	Samza	Spark
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.

Also fix these so that they typecheck correctly without relying on java runtime duck typing.

dpcollins-google · 2019-12-28T06:09:59Z

+R: @boyuanzz

aaltay · 2020-01-03T18:50:25Z

R: @chamikaramj could you find a person to review this PR?

boyuanzz · 2020-01-07T02:50:09Z

@chamikaramj @ihji
Hey Cham and Heejong, could one of you please help review this PR? Major changes are related to build Pubsub source and sink as cross language IO

chamikaramj · 2020-01-07T02:53:45Z

Cloud you create JIRAs and (break up PR as needed) to clarify context. Also title says cleanup but based on above comment seems like this is a new feature ?

dpcollins-google · 2020-01-07T18:07:21Z

This is cleanup, not a new feature. This only involves 6fd05a7, and moves things that exist in PubsubIO that don't need to be there outside of PubsubIO and make them type check correctly.

chamikaramj · 2020-01-07T18:10:42Z

Thanks. Please still create a JIRA to capture the context of the change and add it to title in the form [BEAM-JIRA#].
Please see following for more details on Beam contribution process.
https://beam.apache.org/contribute/

@ihji can you help with the review ?
cc: @chadrik

chadrik · 2020-01-07T18:46:22Z

LGTM.

cc'ing @mxm since he did the Kafka external transform support.

dpcollins-google · 2020-01-08T03:16:20Z

Just rebased this. It should be clearer now that this is just cleanup.

udim · 2020-01-08T18:21:39Z

Are there any tests for these external reads and writes?

mxm

Also fix these so that they typecheck correctly without relying on java runtime duck typing.

Could you explain what you changed with regards to this?

dpcollins-google · 2020-01-09T16:19:33Z

Also fix these so that they typecheck correctly without relying on java runtime duck typing.
Could you explain what you changed with regards to this?

Previously, these classes were inside a generic context with a parameter type of the builder, but they would have thrown an exception due to a bad cast if they were used with any type other than byte[]. This makes it explicit that the output type of the PTransform is a byte[] and removes some unsafe casts that were only correct if the output type is a byte[].

dpcollins-google · 2020-01-09T16:27:45Z

Retest all please

mxm · 2020-01-09T17:30:14Z

Makes sense. Thanks for addressing this in the cleanup.

chamikaramj · 2020-01-09T18:27:56Z

In general, are changes to PubSubIO in this and other PRs you authored backwards compatible ? If change changes are not backwards compatible (if users cannot upgrade Beam without changes to their pipelines) i think this needs a deeper look since Beam guarantees backwards compatibility across minor versions. This is OK for external transforms part which is experimental but PubSubIO is used in production by many users.

dpcollins-google · 2020-01-09T20:18:27Z

From an api perspective, neither this change nor any of the other ones until #10481 should have any user visible effects. That one is an attempt to add to the user api surface, but not remove anything.

udim · 2020-01-09T21:59:02Z

retest this please

udim · 2020-01-09T23:37:05Z

retest this please

udim · 2020-01-10T00:52:08Z

retest this please

ihji

LGTM.

dpcollins-google · 2020-01-13T14:39:51Z

retest this please

chamikaramj · 2020-01-13T17:15:24Z

retest this please

chamikaramj · 2020-01-13T17:16:19Z

Retest this please

chamikaramj · 2020-01-15T01:04:32Z

Please fix the errors. For example,
https://scans.gradle.com/s/pv2iijqg4o27m/console-log?task=:sdks:java:io:google-cloud-platform:compileJava

dpcollins-google · 2020-02-06T20:51:37Z

Retest this please

chamikaramj · 2020-02-06T20:57:01Z

Retest this please

chamikaramj · 2020-02-07T01:51:02Z

Retest this please

chamikaramj · 2020-02-07T18:27:09Z

Retest this please

dpcollins-google · 2020-02-08T17:35:12Z

I believe the only issue turned up was inclusion of raw non-versioned ImmutableMap. Could we retest this please?

dpcollins-google · 2020-02-08T17:35:22Z

Retest this please

chamikaramj · 2020-02-10T18:07:13Z

Retest this please

chamikaramj · 2020-02-10T21:03:22Z

Retest this please

chamikaramj · 2020-02-10T21:44:28Z

Retest this please

yifanzou · 2020-02-11T18:07:42Z

Run Java PreCommit

…ubsubIO hooks outside of PubsubIO.

Move external PubsubIO hooks outside of PubsubIO.
0a15bca
Also fix these so that they typecheck correctly without relying on java runtime duck typing.

dpcollins-google force-pushed the update-external branch 5 times, most recently from 834d304 to b19859bCompare December 29, 2019 16:22

dpcollins-google force-pushed the update-external branch from b19859b to 6fd05a7Compare January 6, 2020 20:59

dpcollins-google mentioned this pull request Jan 7, 2020
[BEAM-8932][Cleanup] Cleanup pubsubio by removing optionality and adding defaults to builders. #10477
Merged
3 tasks

dpcollins-google changed the title ~~[Cleanup] Move external PubsubIO hooks outside of PubsubIO.~~[BEAM-8932][Cleanup] Move external PubsubIO hooks outside of PubsubIO.Jan 7, 2020

dpcollins-google force-pushed the update-external branch from 6fd05a7 to ce12b9dCompare January 8, 2020 03:15

udim reviewed Jan 8, 2020
View reviewed changes

Comment threadsdks/go/container/boot.go Outdated

mxm reviewed Jan 9, 2020
View reviewed changes

Comment thread...a/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/pubsub/ExternalRead.java Outdated
Comment thread.../io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/pubsub/ExternalWrite.java Outdated

dpcollins-google force-pushed the update-external branch from ce12b9d to 453b973Compare January 9, 2020 16:25

mxm approved these changes Jan 9, 2020
View reviewed changes

dpcollins-google force-pushed the update-external branch from 453b973 to 6078e95Compare January 9, 2020 22:15

dpcollins-google force-pushed the update-external branch from 6078e95 to c7da5a2Compare January 9, 2020 22:31

ihji approved these changes Jan 11, 2020
View reviewed changes

dpcollins-google force-pushed the update-external branch 2 times, most recently from 54a14ae to 3b9156eCompare February 6, 2020 20:51

dpcollins-google force-pushed the update-external branch from 3b9156e to 61f4a96Compare February 8, 2020 17:33

dpcollins-google force-pushed the update-external branch from 61f4a96 to 0a15bcaCompare February 10, 2020 21:00

chamikaramj merged commit 6d721c1 into apache:masterFeb 11, 2020

dpcollins-google pushed a commit to dpcollins-google/beam that referenced this pull request Jan 27, 2021
Merge pull requestapache#10476: [BEAM-8932][Cleanup] Move external P…
b19d240
…ubsubIO hooks outside of PubsubIO.

Uh oh!

Conversation

dpcollins-google commented Dec 28, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Post-Commit Tests Status (on master branch)

Pre-Commit Tests Status (on master branch)

Uh oh!

dpcollins-google commented Dec 28, 2019

Uh oh!

aaltay commented Jan 3, 2020

Uh oh!

boyuanzz commented Jan 7, 2020

Uh oh!

chamikaramj commented Jan 7, 2020

Uh oh!

dpcollins-google commented Jan 7, 2020

Uh oh!

chamikaramj commented Jan 7, 2020

Uh oh!

chadrik commented Jan 7, 2020

Uh oh!

dpcollins-google commented Jan 8, 2020

Uh oh!

Uh oh!

udim commented Jan 8, 2020

Uh oh!

mxm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

dpcollins-google commented Jan 9, 2020

Uh oh!

dpcollins-google commented Jan 9, 2020

Uh oh!

mxm commented Jan 9, 2020

Uh oh!

chamikaramj commented Jan 9, 2020

Uh oh!

dpcollins-google commented Jan 9, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

udim commented Jan 9, 2020

Uh oh!

udim commented Jan 9, 2020

Uh oh!

udim commented Jan 10, 2020

Uh oh!

ihji left a comment

Choose a reason for hiding this comment

Uh oh!

dpcollins-google commented Jan 13, 2020

Uh oh!

chamikaramj commented Jan 13, 2020

Uh oh!

chamikaramj commented Jan 13, 2020

Uh oh!

chamikaramj commented Jan 15, 2020

Uh oh!

dpcollins-google commented Feb 6, 2020

Uh oh!

chamikaramj commented Feb 6, 2020

Uh oh!

chamikaramj commented Feb 7, 2020

Uh oh!

chamikaramj commented Feb 7, 2020

Uh oh!

dpcollins-google commented Feb 8, 2020

Uh oh!

dpcollins-google commented Feb 8, 2020

Uh oh!

chamikaramj commented Feb 10, 2020

Uh oh!

chamikaramj commented Feb 10, 2020

Uh oh!

chamikaramj commented Feb 10, 2020

Uh oh!

yifanzou commented Feb 11, 2020

Uh oh!

Reviewers

dpcollins-google commented Dec 28, 2019•
edited
Loading

dpcollins-google commented Jan 9, 2020•
edited
Loading