[BEAM-8388] Make sdk-java-core compatible with avro 1.8 & 1.9 by RustedBones · Pull Request #16271 · apache/beam

RustedBones · 2021-12-17T09:59:35Z

Beam can run with different versions of avro at runtime. However, some fixes from avro 1.9 are not visible due to beam internals

AVRO-1891 is fixed by adding the conversion at compile time in the generated class' model. Beam does not currently read the SpecificData form the class.
BEAM-8329 ReflectData is not supposed to use the logical conversions by default. Users should either use a CustomEncoding with the AvroEncode annotation or manually add the conversion to their own ReflectData which is not possible everywhere in beam.
BEAM-9144 is a 'snowflake'. All other joda logical time conversions are still failing. (date, time-millis, time-micros, timestampt-micros). This should not be in the framework but in the user code

This PR aims to

Keep both avro 1.8 and 1.9 support
Use proper logical type conversions when using avro 1.9 specific data (useReflectApi must be disabled)
Give users the possibility to provide their own avro data in the DatumWriterFactory and DatumReaderFactory used by beam internals

Needs discussion:

AvroCoder should have constructor with DatumWriterFactory and DatumReaderFactory but this causes a serialization issue with the AvroCoderCloudObjectTranslator whenre the object needs to be converted into a json form.

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.

`ValidatesRunner` compliance status (on master branch)

Lang	ULR	Twister2
Go	---	---
Java
Python	---	---
XLang		---

Examples testing status on various runners

Lang	ULR	Dataflow	Flink	Samza	Spark	Twister2
Go	---	---	---	---	---	---	---
Java	---		---	---	---	---	---
Python	---	---	---	---	---	---	---
XLang	---	---	---	---	---	---	---

Post-Commit SDK/Transform Integration Tests Status (on master branch)

Go	Java	Python

Pre-Commit Tests Status (on master branch)

---	Java	Python	Go	Website	Whitespace	Typescript
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.

RustedBones · 2021-12-17T10:06:30Z

This is a breaking change. The class needs to be propagated for proper factories instanciation.

Can you make it into two codepaths? I am just coming to this PR to help out a little so I don't have all the history of your thoughts on this.

I guess it was a question if we can avoid breaking changes here, isn't it?

RustedBones · 2021-12-17T10:08:00Z

This is a breaking change. It aligns better with the Source implementation.

Can you make this also a separate code path?

RustedBones · 2021-12-17T15:25:32Z

R: @aromanenko-dev

aromanenko-dev · 2021-12-20T14:14:22Z

Thanks! I'm a bit busy with other PRs for the moment but I'll try to take a look on this in the next days.

CC: @RyanSkraba @iemejia fyi, if you have some time, ptal too

aromanenko-dev · 2021-12-27T14:44:15Z

@RustedBones Sorry, I still didn't have a time for review but did you check a failed test? Is it related to your changes?

RustedBones · 2022-01-03T16:15:40Z

@aromanenko-dev yes, this is related to my changes. It has some impact on the readAvrosWithBeamSchema.

To me, the only way to properly enable logical-type support with avro reflect classes is to create an AvroCoder with custom factories

aromanenko-dev · 2022-01-04T14:34:30Z

CC: @kennknowles @lukecwik @TheNeuralBit
I think that this PR brings some significant changes to AvroCoder and it should be discussed before.

TheNeuralBit · 2022-01-05T00:52:11Z

Is it making breaking changes to AvroCoder?

RustedBones · 2022-01-07T14:34:53Z

There are no breaking changes to the AvroCoder directly.
I'd like to add the possibility to create coders using datum factories. Users could customize (de)serialization and enable more advanced avro features (eg. BEAM-9144 to get reader and writer supporting both java.time and joda.time)

The breaking changes are:

Some AvroIO API requiring to pass class type
AvroCoderCloudObjectTranslator is not able so serialize custom factories

Maybe we should do as for the AvroGenericCoder and give specialized implementation for the AvroSpecificCoder and AvroReflectCoder. Change visibility of the AvroCoder so users can extend and create their own.
Only provide the CloudObjectTranslator, for AvroGenericCoder, AvroSpecificCoder and AvroReflectCoder and let users provider their own translator if they use a custom avro coder

kennknowles · 2022-01-10T20:35:05Z

Can you make it into two codepaths? I am just coming to this PR to help out a little so I don't have all the history of your thoughts on this.

kennknowles · 2022-01-10T20:35:42Z

Can you make this also a separate code path?

kennknowles · 2022-01-10T20:37:00Z

nit: a class is slightly different from a type (types have generics and constraints, etc, etc) so prefer some identifier like elementClass or clazz

I know but this has been called 'type' everywhere in the Avro section (AvroCoder & AvroWriter and AvroSource.Mode). I'm just keeping the consistency

aaltay · 2022-01-27T02:04:20Z

What is the next step on this PR?

RustedBones · 2022-01-31T09:38:12Z

On my side, I'd say the PR is ready and addresses the main pain points.
There are maybe some things to consider on the beam maintainer side:

there are some small breaking changes in the avro API.
timestamp-millis logical type conversion is not supported by default anymore and will require custom factories & coder.

RustedBones · 2022-03-02T17:02:43Z

Any plans on how to proceed with this ?

aaltay · 2022-03-02T18:52:40Z

pinging @kennknowles for the above question.

aromanenko-dev

@RustedBones
Thank you for your patience, I'm sorry for delay with a response and I have several questions:

What are the actual breaking changes? Could you list them?
Are all review comments already addressed and, if not, could you elaborate why?
Could you squash all commits and rebase your feature branch against master instead of merge it?

RustedBones · 2022-03-07T14:07:18Z

Sure @aromanenko-dev!

What are the actual breaking changes? Could you list them?

API breaking changes:

AvroCoder: removing useReflectApi() which was introduced in 2.36.0 in favour of specialized implementations
~~AvroIO: writeCustomType now must be provided with the class of the output record~~
JodaTimestampConversion is removed

Runtime breaking changes:

Current avro coder uses a custom ReflectData, initialized with JodaTimestampConversion. This PR will remove this and only use default avro's GenericData, SpecificData or ReflectData.
Any users that are using avro 1.9 would need to manually add joda conversion to be able to support schemas compiled with avro 1.8 (time logical types have been migrated to java.time).
Any users at use reflect data with org.joda.time.DateTime will have to either use the AvroEncode annotation or manually add joda conversion as done here

Are all review comments already addressed and, if not, could you elaborate why?

This comment and this one are not answered. I'm not sure I understood properly. I think the generic codepath is already covered by writeCustomTypeToGenericRecords in the 1st case. In the 2nd case the API aligns with the Read ones.

aromanenko-dev · 2022-03-09T15:36:00Z

Run Java PreCommit

aromanenko-dev

Many thanks for details!

Can we avoid these or most of the breaking changes? I'd prefer to deprecate it and add a new functionality at first then just remove it?

aromanenko-dev · 2022-03-09T17:35:40Z

Why do we need this be public?

Constructor need to be accessed from AvroCoder and users should be able to extends the class to create their own coder

aromanenko-dev · 2022-03-09T17:37:29Z

Why do we need public constructors?

aromanenko-dev · 2022-03-09T17:39:05Z

I guess it was a question if we can avoid breaking changes here, isn't it?

Unifies how AvroCoder and AvroIO works with datum reader and writer so that clients can either use Avro 1.8 or 1.9 without changing base behaviour and limited breaking changes.

aaltay · 2022-04-28T17:52:17Z

Is this still in progress? Could this be closed?

RustedBones · 2022-04-29T11:01:40Z

I've addressed the last comments. Is there anything else you'd want to be improved ?
I still think this is a relevant change to enable users to move to avro 1.9 if they want to. Then it is up to the maintainer to see if it is fine to:

drop the timestamp-millis logical type conversion on avro reflect (will require custom factories and coders).
Avro 1.9 users should manually add the Joda DateTime conversion since Beam's copy is removed

aaltay · 2022-05-12T21:40:45Z

Friendly ping @aromanenko-dev to take another look.

aromanenko-dev · 2022-05-13T15:34:07Z

I'm sorry for not being clear from the beginning (for my excuse, I was a little bit out of the context) but since this topic (Avro version update in Beam) seems doesn't have an obvious and strait-forward solution for now, I started a discussion on dev@beam.apache.org to decide how we should move forward with this.

RustedBones · 2022-10-13T14:45:52Z

Did you come to any decision concerning Avro's future in beam ?

aromanenko-dev · 2022-10-13T15:21:10Z

@RustedBones Unfortunately, there is no perfect solution for that. We are working on design doc that should help us to move forward. I hope to send it to dev@beam.apache.org soon.

RustedBones · 2023-03-01T12:20:04Z

@aromanenko-dev should some of the bug fixes be ported into the new avro extension module ?

aromanenko-dev · 2023-03-06T15:38:35Z

@RustedBones Since starting from Beam 2.46.0 extensions/avro is a default way to use Avro in Beam, then I'd say that we have to apply the bug fixes in the first order to this extension and, optionally, to old deprecated code in core

aromanenko-dev · 2023-03-06T15:40:57Z

@RustedBones Would you mind to close this PR because of transition to extensions/avro?

RustedBones · 2023-03-07T16:25:58Z

Sounds good. I'll move the changes to the new dedicated extension module.

aromanenko-dev · 2023-03-07T16:31:51Z

Thanks, @RustedBones! I'd be happy to review them

RustedBones commented Dec 17, 2021
View reviewed changes

RustedBones mentioned this pull request Dec 17, 2021
Avro logical types support spotify/scio#4101
Closed

aaltay requested a review from aromanenko-dev December 17, 2021 16:31

kennknowles reviewed Jan 10, 2022
View reviewed changes

RustedBones force-pushed the BEAM-8388-prepare-avro-update branch 2 times, most recently from 448c998 to 548183cCompare January 14, 2022 16:02

kennknowles reviewed Jan 14, 2022
View reviewed changes

Comment threadsdks/java/core/src/main/java/org/apache/beam/sdk/schemas/AvroRecordSchema.java Outdated

kennknowles reviewed Jan 14, 2022
View reviewed changes

Comment threadsdks/java/core/src/main/java/org/apache/beam/sdk/schemas/AvroRecordSchema.java Outdated

aromanenko-dev reviewed Mar 4, 2022
View reviewed changes

RustedBones force-pushed the BEAM-8388-prepare-avro-update branch from 26261ec to 5326358Compare March 7, 2022 14:20

github-actionsBot added dataflow gcp io java runners labels Mar 7, 2022

aromanenko-dev requested changes Mar 9, 2022
View reviewed changes

[BEAM-8388] Make sdk-java-core compatible with avro 1.8 & 1.9
2b95f8c
Unifies how AvroCoder and AvroIO works with datum reader and writer so that clients can either use Avro 1.8 or 1.9 without changing base behaviour and limited breaking changes.

Deprecate API instead of breaking
62c0b05

RustedBones force-pushed the BEAM-8388-prepare-avro-update branch from 5326358 to 62c0b05Compare April 15, 2022 09:17

aromanenko-dev mentioned this pull request Apr 15, 2022
[WIP][BEAM-8715] Bump Avro version to 1.9.2 #17372
Closed
4 tasks

RustedBones mentioned this pull request Aug 12, 2022
AvroUtils is converting incorrectly LogicalType Timestamps from long into Joda DateTimes #20205
Open

yyy1000 mentioned this pull request Dec 22, 2022
Zstandard codec for Avro #21218
Closed

RustedBones closed this Mar 7, 2023

RustedBones mentioned this pull request Apr 18, 2023
Select dedicated avro datum reader and writer #26320
Merged

Uh oh!

Conversation

RustedBones commented Dec 17, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

ValidatesRunner compliance status (on master branch)

Examples testing status on various runners

Post-Commit SDK/Transform Integration Tests Status (on master branch)

Pre-Commit Tests Status (on master branch)

GitHub Actions Tests Status (on master branch)

Uh oh!

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!

RustedBones commented Dec 17, 2021

Uh oh!

aromanenko-dev commented Dec 20, 2021

Uh oh!

aromanenko-dev commented Dec 27, 2021

Uh oh!

RustedBones commented Jan 3, 2022

Uh oh!

aromanenko-dev commented Jan 4, 2022

Uh oh!

TheNeuralBit commented Jan 5, 2022

Uh oh!

RustedBones commented Jan 7, 2022

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

aaltay commented Jan 27, 2022

Uh oh!

RustedBones commented Jan 31, 2022

Uh oh!

RustedBones commented Mar 2, 2022

Uh oh!

aaltay commented Mar 2, 2022

Uh oh!

aromanenko-dev left a comment

Choose a reason for hiding this comment

Uh oh!

RustedBones commented Mar 7, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aromanenko-dev commented Mar 9, 2022

Uh oh!

aromanenko-dev left a comment

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!

aaltay commented Apr 28, 2022

Uh oh!

RustedBones commented Apr 29, 2022

Uh oh!

aaltay commented May 12, 2022

Uh oh!

RustedBones commented Dec 17, 2021•
edited
Loading

`ValidatesRunner` compliance status (on master branch)

RustedBones commented Mar 7, 2022•
edited
Loading