[YAML] Fix error handling for KafkaSchemaTransforms by Polber · Pull Request #29261 · apache/beam

Polber · 2023-11-01T23:58:41Z

Add proper error handling to KafkaReadSchemaTransform and KafkaWriteSchemaTransform

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.

Polber · 2023-11-01T23:58:49Z

R: @robertwb

github-actions · 2023-11-02T00:00:16Z

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

Polber · 2023-11-02T00:02:48Z

R: @damccorm

robertwb

Thanks.

...va/io/kafka/src/main/java/org/apache/beam/sdk/io/kafka/KafkaReadSchemaTransformProvider.java

robertwb · 2023-11-02T00:06:52Z

...va/io/kafka/src/main/java/org/apache/beam/sdk/io/kafka/KafkaReadSchemaTransformProvider.java

Hmm... Can we refactor this so as not to be so duplicative here?

I did a quick refactor. There is probably a more elegant solution, but I'm trying to get this submitted asap with 2.52

...va/io/kafka/src/main/java/org/apache/beam/sdk/io/kafka/KafkaReadSchemaTransformProvider.java

...a/io/kafka/src/main/java/org/apache/beam/sdk/io/kafka/KafkaWriteSchemaTransformProvider.java

Signed-off-by: Jeffrey Kinard <jeff@thekinards.com>

codecov · 2023-11-02T01:14:35Z

Codecov Report

Merging #29261 (49d2ee7) into master (e03988e) will decrease coverage by 0.04%.
Report is 21 commits behind head on master.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master   #29261      +/-   ##
==========================================
- Coverage   38.31%   38.28%   -0.04%     
==========================================
  Files         688      689       +1     
  Lines      101879   101990     +111     
==========================================
+ Hits        39040    39043       +3     
- Misses      61259    61367     +108     
  Partials     1580     1580

Flag	Coverage Δ
python	`29.88% <0.00%> (-0.05%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
sdks/python/apache_beam/yaml/yaml_transform.py	`0.00% <0.00%> (ø)`

... and 6 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Signed-off-by: Jeffrey Kinard <jeff@thekinards.com>

sdks/python/apache_beam/yaml/yaml_transform.py

...va/io/kafka/src/main/java/org/apache/beam/sdk/io/kafka/KafkaReadSchemaTransformProvider.java

sdks/python/apache_beam/yaml/yaml_transform.py

robertwb · 2023-11-02T17:03:12Z

...va/io/kafka/src/main/java/org/apache/beam/sdk/io/kafka/KafkaReadSchemaTransformProvider.java

          PCollection<byte[]> kafkaValues =
              input.getPipeline().apply(kafkaRead.withoutMetadata()).apply(Values.create());

+          Schema errorSchema = ErrorHandling.errorSchema(ERROR_SCHEMA);


This probably shouldn't be an ErrorSchema of ERROR_SCHEMA.

We should add a errorSchema(FieldType) overload, where bytes is the thing to pass here. I know John Casey is looking at DLQ stuff where we might want to refactor this a bit more. (I suppose we're calling YAML stable, but the formatting of the error messages may have to change.)

Ok, I did that initially, but I wan't sure if that would mess with our Row-based approach. I can refactor that and test

#29274 (as this is in the docs, no need to cherry-pick that one).

@robertwb How does it look now?

Signed-off-by: Jeffrey Kinard <jeff@thekinards.com>

github-actions bot added python java io kafka yaml labels Nov 1, 2023

robertwb reviewed Nov 2, 2023

View reviewed changes

...a/io/kafka/src/main/java/org/apache/beam/sdk/io/kafka/KafkaWriteSchemaTransformProvider.java Outdated Show resolved Hide resolved

[YAML] Fix error handling for KafkaReadSchemaTransform

470f17f

Signed-off-by: Jeffrey Kinard <jeff@thekinards.com>

Polber force-pushed the jkinard/fix-kafka branch from e6e97f5 to 470f17f Compare November 2, 2023 00:53

Address initial comments

ab1834b

Signed-off-by: Jeffrey Kinard <jeff@thekinards.com>

Polber commented Nov 2, 2023

View reviewed changes

sdks/python/apache_beam/yaml/yaml_transform.py Show resolved Hide resolved

Polber commented Nov 2, 2023

View reviewed changes

...va/io/kafka/src/main/java/org/apache/beam/sdk/io/kafka/KafkaReadSchemaTransformProvider.java Outdated Show resolved Hide resolved

Polber requested a review from robertwb November 2, 2023 05:26

robertwb reviewed Nov 2, 2023

View reviewed changes

Polber force-pushed the jkinard/fix-kafka branch from 5ecf053 to fc607b4 Compare November 2, 2023 19:05

Polber requested a review from robertwb November 2, 2023 19:57

refactor row errors to byte array errors

49d2ee7

Signed-off-by: Jeffrey Kinard <jeff@thekinards.com>

Polber force-pushed the jkinard/fix-kafka branch from fc607b4 to 49d2ee7 Compare November 2, 2023 21:21

robertwb approved these changes Nov 3, 2023

View reviewed changes

robertwb merged commit 07d5d53 into apache:master Nov 3, 2023

Polber added a commit to Polber/beam that referenced this pull request Nov 3, 2023

[YAML] Fix error handling for KafkaSchemaTransforms (apache#29261)

25daf4e

Polber mentioned this pull request Nov 3, 2023

[YAML] Fix error handling for KafkaSchemaTransforms (#29261) #29289

Merged

3 tasks

damccorm pushed a commit that referenced this pull request Nov 3, 2023

[YAML] Fix error handling for KafkaSchemaTransforms (#29261) (#29289)

0e8e54c

Conversation

Polber commented Nov 1, 2023

GitHub Actions Tests Status (on master branch)

Uh oh!

Polber commented Nov 1, 2023

Uh oh!

github-actions bot commented Nov 2, 2023

Uh oh!

Polber commented Nov 2, 2023

Uh oh!

robertwb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

robertwb Nov 2, 2023

Choose a reason for hiding this comment

Uh oh!

Polber Nov 2, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Nov 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

robertwb Nov 2, 2023

Choose a reason for hiding this comment

Uh oh!

Polber Nov 2, 2023

Choose a reason for hiding this comment

Uh oh!

robertwb Nov 2, 2023

Choose a reason for hiding this comment

Uh oh!

Polber Nov 2, 2023

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codecov bot commented Nov 2, 2023 •

edited

Loading