Fixes breakages of the upgrade feature by chamikaramj · Pull Request #29731 · apache/beam

chamikaramj · 2023-12-12T23:23:21Z

Seems like this feature broke due to.

Object serialization issues (specifically BQ coder and BigQueryServicesImpl).
A new field addition to KafkaIO.

Makes the implementation more robust and adds the Kafka upgrade module to IO precommit.

This fixes #29730

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

Mention the appropriate issue in your description (for example: addresses #123), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, comment fixes #<ISSUE NUMBER> instead.
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.

To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md

GitHub Actions Tests Status (on master branch)

See CI.md for more information about GitHub Actions CI or the workflows README to see a list of phrases to trigger workflows.

chamikaramj · 2023-12-12T23:23:53Z

R: @johnjcasey

github-actions · 2023-12-12T23:25:07Z

Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control

johnjcasey · 2023-12-13T16:12:36Z

...-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryIOTranslation.java

it doesn't look like you actually set the default coder here

The default coder is set by the source transform if I don't set anything here.

beam/sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryIO.java

Line 1247 in 951b3b1

final Coder<T> coder = inferCoder(p.getCoderRegistry());

johnjcasey · 2023-12-13T16:13:01Z

...-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryIOTranslation.java

How would one know which fields could fail to deserialize?

I mentioned the field in the log (and should also be in the exception stacktrace). I only handle specific cases here. If deserialization fails for any other cases, it would be a hard fail during job submission.

johnjcasey · 2023-12-13T16:13:25Z

...-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryIOTranslation.java

If this is sufficient for correct behavior, why do we deserialize the service bytes at all?

Unfortunately this object is set by default for BQ source/sink transforms and BQ transforms cannot be constructed without it. See below.

beam/sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryIO.java

Line 724 in 951b3b1

.setBigQueryServices(new BigQueryServicesImpl())

beam/sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryIO.java

Line 2127 in 951b3b1

.setBigQueryServices(new BigQueryServicesImpl())

So I have to set it to re-build source/sink transform object here.

johnjcasey · 2023-12-13T16:18:58Z

.../io/kafka/upgrade/src/main/java/org/apache/beam/sdk/io/kafka/upgrade/KafkaIOTranslation.java

the transform behavior changes whether or not an error handler is passed at all, so this could break if deserialization fails.

I think ErrorHandler has to be instantiated with it's own schema but will do this in a separate PR.
For now, updating the code so that we do a hard fail if the user set this property.

chamikaramj

Thanks. PTAL.

chamikaramj · 2023-12-13T17:23:10Z

...-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryIOTranslation.java

I mentioned the field in the log (and should also be in the exception stacktrace). I only handle specific cases here. If deserialization fails for any other cases, it would be a hard fail during job submission.

chamikaramj · 2023-12-13T17:25:40Z

...-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryIOTranslation.java

Unfortunately this object is set by default for BQ source/sink transforms and BQ transforms cannot be constructed without it. See below.

beam/sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryIO.java

Line 724 in 951b3b1

.setBigQueryServices(new BigQueryServicesImpl())

beam/sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryIO.java

Line 2127 in 951b3b1

.setBigQueryServices(new BigQueryServicesImpl())

So I have to set it to re-build source/sink transform object here.

chamikaramj · 2023-12-13T17:26:59Z

...-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryIOTranslation.java

The default coder is set by the source transform if I don't set anything here.

beam/sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryIO.java

Line 1247 in 951b3b1

final Coder<T> coder = inferCoder(p.getCoderRegistry());

chamikaramj · 2023-12-13T19:05:29Z

.../io/kafka/upgrade/src/main/java/org/apache/beam/sdk/io/kafka/upgrade/KafkaIOTranslation.java

I think ErrorHandler has to be instantiated with it's own schema but will do this in a separate PR.
For now, updating the code so that we do a hard fail if the user set this property.

johnjcasey · 2023-12-13T20:10:07Z

LGTM for the cherry pick. We should brainstorm how to make error handling more robust going forward

chamikaramj · 2023-12-13T22:17:43Z

Thanks.

chamikaramj · 2023-12-14T01:22:54Z

I don't think PreComit failures are related to this PR.

@Abacn @jrmccluskey I noticed that there are ongoing efforts to fix PreCommit test suites (for example, #28957, #29671). Should there be a release blocker for fixing the PreCommit to perform the release in a healthy state ?

* Fixes breakages of the upgrade feature * Fix spotless * Addressing reviewer comments * Removing unused import * Reverting the PreCommit update

Abacn · 2023-12-14T13:02:16Z

#29744 should reduce flakiness of Java PreCommit on master

* Fixes breakages of the upgrade feature * Fix spotless * Addressing reviewer comments * Removing unused import * Reverting the PreCommit update

github-actions bot added java build io runners gcp kafka core labels Dec 12, 2023

chamikaramj mentioned this pull request Dec 12, 2023

[Bug]: Transform upgrade feature is broken due to a recent field addition and object serialization issues #29730

Closed

16 tasks

johnjcasey requested changes Dec 13, 2023

View reviewed changes

chamikaramj commented Dec 13, 2023

View reviewed changes

chamikaramj added 4 commits December 13, 2023 15:42

Fixes breakages of the upgrade feature

f9483c7

Fix spotless

f8beef2

Addressing reviewer comments

728e879

Removing unused import

8ba9ab4

chamikaramj force-pushed the transform_upgrade_test_bq_fix branch from 3b2110d to 8ba9ab4 Compare December 13, 2023 23:43

Reverting the PreCommit update

4584043

github-actions bot removed the build label Dec 14, 2023

chamikaramj merged commit 4264c2c into apache:master Dec 14, 2023

chamikaramj mentioned this pull request Dec 14, 2023

Fixes breakages of the upgrade feature (#29731) #29766

Merged

3 tasks

jrmccluskey pushed a commit that referenced this pull request Dec 15, 2023

Fixes breakages of the upgrade feature (#29731) (#29766)

667027d

* Fixes breakages of the upgrade feature * Fix spotless * Addressing reviewer comments * Removing unused import * Reverting the PreCommit update

AnandInguva mentioned this pull request Dec 15, 2023

[Task]: Fix typos in sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryIOTranslation.java #29783

Closed

16 tasks

Conversation

chamikaramj commented Dec 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

GitHub Actions Tests Status (on master branch)

Uh oh!

chamikaramj commented Dec 12, 2023

Uh oh!

github-actions bot commented Dec 12, 2023

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!

chamikaramj 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!

johnjcasey commented Dec 13, 2023

Uh oh!

chamikaramj commented Dec 13, 2023

Uh oh!

chamikaramj commented Dec 14, 2023

Uh oh!

Abacn commented Dec 14, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

chamikaramj commented Dec 12, 2023 •

edited

Loading