Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Shade updated AvroCoder until Beam 2.33.0 is released #3957

Merged
merged 2 commits into from
Aug 31, 2021

Conversation

clairemcginty
Copy link
Contributor

Since the AvroCoder String ser/de fix isn't going into Beam 2.32.0, this PR shades that updated AvroCoder until Beam 2.33 comes out and we can release it. This change only affects <: SpecificRecordBase Avro types, so no updates to the Generic Avro coders are required.

@codecov
Copy link

codecov bot commented Aug 25, 2021

Codecov Report

Merging #3957 (2b5d364) into main (73d731d) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #3957   +/-   ##
=======================================
  Coverage   62.05%   62.05%           
=======================================
  Files         286      286           
  Lines       10220    10220           
  Branches      379      379           
=======================================
  Hits         6342     6342           
  Misses       3878     3878           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 73d731d...2b5d364. Read the comment docs.

@@ -0,0 +1,36 @@
/*
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we have Spotify or ASF license header here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess ASF since the code is just copied from Beam.

Copy link
Contributor

@stormy-ua stormy-ua left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚢

Copy link
Contributor

@regadas regadas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have some concerns around this since this is a breaking a change.

We can go ahead with this but this means that the next release of scio should be v0.11.0 and it should include beam v2.32.0.

@clairemcginty
Copy link
Contributor Author

clairemcginty commented Aug 26, 2021

I have some concerns around this since this is a breaking a change.

We can go ahead with this but this means that the next release of scio should be v0.11.0 and it should include beam v2.32.0.

@regadas Yeah the idea would be to release it with beam 2.32. It shouldn’t be a breaking change, I would consider it a bug fix which part did you mean specifically?

@regadas
Copy link
Contributor

regadas commented Aug 26, 2021

@clairemcginty this is source compat but it's a binary incompatible change the underlying class is different; this means that if users pull different scio patch versions they would probably get ClassNotFoundException.

The reason I asked about bringing beam v2.32.0 it's because we check for the same beam version; users will need to fix the dependencies bringing everything to the same scio version; If we didn't bump it users would face issues;

This is a side effect that it's not obvious so I recommend we really target this to v0.11.0.

@stormy-ua
Copy link
Contributor

stormy-ua commented Aug 26, 2021

We also have one additional outstanding PR with a breaking change. Might be a good idea to include both and just release scio 0.11, wdyt? And we definitely want to have beam 2.32 in the next release bc there are some other things we need from 2.32.

@regadas
Copy link
Contributor

regadas commented Aug 26, 2021

There more PR's for v0.11.0; Btw what specifically do we need from beam v2.32.0?

@stormy-ua
Copy link
Contributor

stormy-ua commented Aug 26, 2021

We want to get 2.32 to try bumping gcs-connector up to 2.2.2 to get around Flink-parquet issue. This PR is included into 2.32 - apache/beam#14817. This also means we will need some time to validate gcs-connector 2.2.2 on the scio side (1-2 days?) before making 0.11 release.

@regadas
Copy link
Contributor

regadas commented Aug 26, 2021

@stormy-ua 👍 ahhh yes that one I forgot about it!

@stormy-ua
Copy link
Contributor

stormy-ua commented Aug 26, 2021

yeah, but this would also slightly delay scio 0.11 release, because we will need to do some validations on the scio side and we can start doing this only once beam 2.32 is released which isn't released yet as of now 🤷

@clairemcginty
Copy link
Contributor Author

clairemcginty commented Aug 26, 2021

@clairemcginty this is source compat but it's a binary incompatible change the underlying class is different; this means that if users pull different scio patch versions they would probably get ClassNotFoundException.

The reason I asked about bringing beam v2.32.0 it's because we check for the same beam version; users will need to fix the dependencies bringing everything to the same scio version; If we didn't bump it users would face issues;

This is a side effect that it's not obvious so I recommend we really target this to v0.11.0.

@regadas really? I copied the AvroCoders into org.apache.beam.sdk.coders.shaded package -- not their package in Beam which is just org.apache.beam.sdk.coders -- thought that would avoid the binary incompat problem as they're not overlapping existing classes.

@nevillelyh nevillelyh added this to the 0.11.0 milestone Aug 30, 2021
@regadas regadas merged commit 5e47568 into main Aug 31, 2021
@regadas regadas deleted the claire/avro_coder_shaded branch August 31, 2021 09:23
@regadas regadas added the bug Something isn't working label Aug 31, 2021
regadas pushed a commit to regadas/scio that referenced this pull request Sep 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants