Skip to content

[BEAM-13341] Serialize reflection parameter in AvroCoder cloud object - #16080

Merged
aromanenko-dev merged 7 commits into
apache:masterfrom
RustedBones:BEAM-13341-avro-coder-cloud-object-reflect
Dec 15, 2021
Merged

[BEAM-13341] Serialize reflection parameter in AvroCoder cloud object#16080
aromanenko-dev merged 7 commits into
apache:masterfrom
RustedBones:BEAM-13341-avro-coder-cloud-object-reflect

Conversation

@RustedBones

@RustedBonesRustedBones commented Nov 29, 2021

Copy link
Copy Markdown
Contributor

Since #15616 a new parameter has been added to construct AvroCoders.
This option is not serialized in the could object and thus does not allow to use avro coders with non-default option for 'useReflectApi' in dataflow


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)

LangULRDataflowFlinkSamzaSparkTwister2
Go---Build StatusBuild StatusBuild StatusBuild Status---
JavaBuild StatusBuild Status
Build Status
Build Status
Build Status
Build Status
Build Status
Build Status
Build Status
Build Status
Build Status
Build Status
Build Status
Build Status
Build Status
Build Status
Python---Build Status
Build Status
Build Status
Build Status
Build Status
Build StatusBuild Status---
XLangBuild StatusBuild StatusBuild StatusBuild StatusBuild Status---

Examples testing status on various runners

LangULRDataflowFlinkSamzaSparkTwister2
Go---------------------
Java---Build Status
Build Status
Build Status
---------------
Python---------------------
XLang---------------------

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

GoJavaPython
Build StatusBuild StatusBuild Status
Build Status
Build Status

Pre-Commit Tests Status (on master branch)

---JavaPythonGoWebsiteWhitespaceTypescript
Non-portableBuild Status
Build Status
Build Status
Build Status
Build Status
Build StatusBuild StatusBuild StatusBuild Status
Portable---Build StatusBuild Status---------

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

GitHub Actions Tests Status (on master branch)

Build python source distribution and wheels
Python tests
Java tests

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

@RustedBones

Copy link
Copy Markdown
ContributorAuthor

@aromanenko-dev

@RustedBones

RustedBones commented Dec 1, 2021

Copy link
Copy Markdown
ContributorAuthor

Failing test is passing on my local setup. Jenkins failed with

Error Message
org.junit.runners.model.TestTimedOutException: test timed out after 6000 milliseconds

Is it a flaky test? Can this be re-triggered ?

@RustedBones

Copy link
Copy Markdown
ContributorAuthor

@lukecwik & @pabloem

@aromanenko-dev

Copy link
Copy Markdown
Contributor

Run Java PreCommit

@aromanenko-dev

Copy link
Copy Markdown
Contributor

@RustedBones Looks like that a failed test is not related to your change

@RustedBones

Copy link
Copy Markdown
ContributorAuthor

Thanks @aromanenko-dev !
What would be the next step ? Should I update the CHANGES.md for this bug fix ?
This is my 1st PR on the beam project.

@aromanenko-devaromanenko-dev left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks! This change makes sense for me but I'd ask someone who supports a Dataflow runner to take a look on this. @lukecwik@reuvenlax could you take a look or ping someone else, please?

CHANGES.md is not required to update, just in case you want to highlight your change in release notes.

Comment threadsdks/java/core/src/main/java/org/apache/beam/sdk/coders/AvroCoder.java Outdated
Comment threadsdks/java/core/src/main/java/org/apache/beam/sdk/coders/AvroCoder.java Outdated
@aromanenko-dev

Copy link
Copy Markdown
Contributor

Run Java_Examples_Dataflow PreCommit

@aromanenko-devaromanenko-dev left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM, just a small note. ptal

Comment threadsdks/java/core/src/main/java/org/apache/beam/sdk/coders/AvroCoder.java Outdated
@RustedBones

Copy link
Copy Markdown
ContributorAuthor

I found some other problems with the AvroCoder.
This should now have the expected behaviour.

Also as commented in BEAM-2303:

It probably needs similarly be implemented in AvroIO

I'll prepare another PR for this. It should come with a better support of avro 1.9 when used at runtime

@aromanenko-devaromanenko-dev left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks, LGTM

@aromanenko-dev
aromanenko-dev merged commit bc82f6f into apache:masterDec 15, 2021
@RustedBones
RustedBones deleted the BEAM-13341-avro-coder-cloud-object-reflect branch December 15, 2021 17:30
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@RustedBones@aromanenko-dev