Feature/dead letter queue core #29164

johnjcasey · 2023-10-27T15:27:53Z

Add the base utilities for creating error handlers and adding DLQs to Java IOs

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.

# Conflicts: # CHANGES.md

Create gradle task and github actions config for GCS using this.

…tests Feature/automate performance tests

…tests fix call to gradle

…tests run on hosted runner for testing

…tests add additional checkout

…tests add destination for triggered tests

…tests move env variables to correct location

…tests try uploading against separate dataset

…tests try without a user

…tests update branch checkout, try to view the failure log

…tests run on failure

…tests update to use correct BigQuery instance

…tests convert to matrix

sdks/java/core/src/main/java/org/apache/beam/sdk/transforms/errorhandling/ErrorHandler.java

johnjcasey · 2023-11-16T19:13:18Z

Run Java PreCommit

robertwb · 2023-11-22T19:16:31Z

sdks/java/core/src/main/java/org/apache/beam/sdk/transforms/errorhandling/BadRecord.java

+  public abstract static class Record implements Serializable {
+
+    /** The failing record, encoded as JSON. Will be null if serialization as JSON fails. */
+    public abstract @Nullable String getJsonRecord();


So, thinking about this, we don't expect all records to be (faithfully) represented in json, but that's not the point. The point is to give the user some idea what the record is without having to scrutinize the bytes.

Should we lengthen this to humanReadableJsonRecord to better capture its intent?

I like that, I'll make that change

robertwb · 2023-11-22T19:18:26Z

sdks/java/core/src/main/java/org/apache/beam/sdk/transforms/errorhandling/BadRecordRouter.java

+        throws Exception {
+      if (exception != null) {
+        throw exception;
+      }


Presumably we should be throwing a generic exception if one is not provided?

Maybe something more directly like "Encountered bad record." Might be good to put (some of?) the record itself in the message as it won't be stored elsewhere.

robertwb · 2023-11-22T19:20:08Z

sdks/java/core/src/main/java/org/apache/beam/sdk/transforms/errorhandling/BadRecordRouter.java

+      Preconditions.checkArgumentNotNull(record);
+      ObjectWriter objectWriter = new ObjectMapper().writer().withDefaultPrettyPrinter();
+
+      // Build up record information


I wonder if this logic would be better placed as factory methods on BadRecord itself.

I was thinking the whole block down to line 92.

robertwb · 2023-11-22T19:23:37Z

sdks/java/core/src/main/java/org/apache/beam/sdk/transforms/errorhandling/ErrorHandler.java

+
+    @Override
+    public void close() {
+      closed = true;


Should it be an error (or no-op?) to close this twice?

absolutely yes

robertwb · 2023-11-22T19:24:52Z

sdks/java/core/src/main/java/org/apache/beam/sdk/transforms/errorhandling/ErrorHandler.java

+      closed = true;
+      PCollection<ErrorT> flattened;
+      if (errorCollections.isEmpty()) {
+        LOG.warn("Empty list of error pcollections passed to ErrorHandler.");


Warn is probably too strong, and likely not very actionable.

robertwb · 2023-11-22T19:35:15Z

sdks/java/core/src/main/java/org/apache/beam/sdk/transforms/errorhandling/ErrorHandler.java

+   * configured an error handler or not.
+   */
+  @Internal
+  class NoOpErrorHandler<ErrorT, OutputT extends POutput> implements ErrorHandler<ErrorT, OutputT> {


NoOp seems to imply that we do nothing with the errors (like /dev/null). What we want here is something that we can call addErrorCollection on when we know that the passed error collection will be empty, but should never be used in as an actual error handler. In particular, if there happens to be something in those collections, that's a bug. (Should this be asserted? Actually, in this case it's fine to call close() and all. It's a NoneShallPassErrorHandler.)

I just can't shake the feeling that this'd all be so much easier if we finish pushing error handling into ParDo itself...

I'll rename this. I agree that I'll probably want to rework this once we update the ParDo itself, but that will take quite a lot of design that we don't have in place yet.

robertwb · 2023-11-22T19:38:06Z

...va/core/src/test/java/org/apache/beam/sdk/transforms/errorhandling/BRHEnabledPTransform.java

+            BigEndianIntegerCoder.of(),
+            new RuntimeException(),
+            "Integer was odd",
+            "NoOpDoFn");


I think we'd have to extract this from the DoFn Context object. (Or, more hacky, leverage the same globals that Counters and logging do.)

sdks/java/core/src/test/java/org/apache/beam/sdk/transforms/errorhandling/ErrorHandlerTest.java

…er-queue-core

johnjcasey · 2023-11-28T18:41:14Z

This should be good to go. The pulsar tests are know to be quite flaky

…er-queue-core # Conflicts: # CHANGES.md

robertwb

Thanks, this looks good. Just some minor comments.

robertwb · 2023-12-05T00:16:15Z

sdks/java/core/src/main/java/org/apache/beam/sdk/transforms/errorhandling/BadRecordRouter.java

+        throws Exception {
+      if (exception != null) {
+        throw exception;
+      }


Maybe something more directly like "Encountered bad record." Might be good to put (some of?) the record itself in the message as it won't be stored elsewhere.

robertwb · 2023-12-05T00:17:01Z

sdks/java/core/src/main/java/org/apache/beam/sdk/transforms/errorhandling/BadRecordRouter.java

+      Preconditions.checkArgumentNotNull(record);
+      ObjectWriter objectWriter = new ObjectMapper().writer().withDefaultPrettyPrinter();
+
+      // Build up record information


I was thinking the whole block down to line 92.

robertwb · 2023-12-05T00:23:53Z

sdks/java/core/src/test/java/org/apache/beam/sdk/transforms/errorhandling/ErrorHandlerTest.java

+  public void testErrorHandlerWithBRHTransform() throws Exception {
+    PCollection<Integer> record = pipeline.apply(Create.of(1, 2, 3, 4));
+    DummySinkTransform<BadRecord> transform = new DummySinkTransform<>();
+    ErrorHandler<BadRecord, PCollection<BadRecord>> eh =


Perhaps use a try/autoclosable context?

This lets us manually get access to the output collection, which is useful in the test context

…er-queue-core

johnjcasey and others added 30 commits July 5, 2023 16:05

Update 2.50 release notes to include new Kafka topicPattern feature

c22b8c1

Merge remote-tracking branch 'origin/master'

f7cf5de

# Conflicts: # CHANGES.md

Merge remote-tracking branch 'origin/master'

ab68ecb

Merge remote-tracking branch 'origin/master'

40ad1d5

Create groovy class for io performance tests

6c9c28d

Create gradle task and github actions config for GCS using this.

delete unnecessary class

520c9d1

fix env call

062de23

Merge pull request #181 from johnjcasey/feature/automate-performance-…

01fc25b

…tests Feature/automate performance tests

fix call to gradle

9c9f86b

Merge pull request #182 from johnjcasey/feature/automate-performance-…

92306fa

…tests fix call to gradle

run on hosted runner for testing

925ce55

Merge pull request #183 from johnjcasey/feature/automate-performance-…

2dcfb70

…tests run on hosted runner for testing

add additional checkout

117ef8b

Merge pull request #184 from johnjcasey/feature/automate-performance-…

1f73cda

…tests add additional checkout

add destination for triggered tests

cb6e01b

Merge pull request #185 from johnjcasey/feature/automate-performance-…

a9e86aa

…tests add destination for triggered tests

move env variables to correct location

8ea6c51

Merge pull request #186 from johnjcasey/feature/automate-performance-…

d8822d7

…tests move env variables to correct location

try uploading against separate dataset

320a4cc

Merge pull request #187 from johnjcasey/feature/automate-performance-…

e89b59e

…tests try uploading against separate dataset

try without a user

1cd4e55

Merge pull request #188 from johnjcasey/feature/automate-performance-…

4473f17

…tests try without a user

update branch checkout, try to view the failure log

4fc5b8e

Merge pull request #189 from johnjcasey/feature/automate-performance-…

706650d

…tests update branch checkout, try to view the failure log

run on failure

59069f2

Merge pull request #190 from johnjcasey/feature/automate-performance-…

7f79b62

…tests run on failure

update to use correct BigQuery instance

6f51976

Merge pull request #191 from johnjcasey/feature/automate-performance-…

e95d920

…tests update to use correct BigQuery instance

convert to matrix

df716cb

Merge pull request #192 from johnjcasey/feature/automate-performance-…

4d8eded

…tests convert to matrix

johnjcasey added 2 commits November 14, 2023 15:02

Address comments

f8c6d8c

Fix test cases, spotless

a1b112c

johnjcasey commented Nov 15, 2023

View reviewed changes

sdks/java/core/src/main/java/org/apache/beam/sdk/transforms/errorhandling/ErrorHandler.java Show resolved Hide resolved

johnjcasey added 4 commits November 15, 2023 15:12

remove flatting without error collections

ad1684a

fix nullness

074faf2

spotless + encoding issues

17bf295

spotless

e2ec57f

johnjcasey added 4 commits November 21, 2023 11:12

throw error when error handler isn't used

8b3f052

add concrete bad record error handler class

525d912

spotless, fix test category

9b4a348

fix checkstyle

d6f4097

robertwb reviewed Nov 22, 2023

View reviewed changes

johnjcasey added 7 commits November 27, 2023 11:16

clean up comments

a067238

fix test case

408bc26

Merge remote-tracking branch 'upstream/master' into feature/dead-lett…

0246a80

…er-queue-core

remove "failing transform" field on bad record, add note to CHANGES.md

ec50bff

fix failing test cases

b092709

fix failing test cases

3f1e97c

apply spotless

4356f27

Merge remote-tracking branch 'upstream/master' into feature/dead-lett…

5067bd1

…er-queue-core # Conflicts: # CHANGES.md

robertwb approved these changes Dec 5, 2023

View reviewed changes

johnjcasey added 4 commits December 5, 2023 10:33

apply final comments

864c429

apply final comments

8307b63

apply final comments

fad9d56

Merge remote-tracking branch 'upstream/master' into feature/dead-lett…

c20c2d7

…er-queue-core

johnjcasey merged commit 22ae2e3 into apache:master Dec 5, 2023
33 checks passed

liferoad mentioned this pull request May 6, 2024

[Feature Request]: Add with_exception_handling() for PTransforms in Python #31193

Open

16 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/dead letter queue core #29164

Feature/dead letter queue core #29164

johnjcasey commented Oct 27, 2023

johnjcasey commented Nov 16, 2023

robertwb Nov 22, 2023

johnjcasey Nov 27, 2023

robertwb Nov 22, 2023

robertwb Dec 5, 2023

robertwb Nov 22, 2023

johnjcasey Nov 27, 2023

robertwb Dec 5, 2023

robertwb Nov 22, 2023

johnjcasey Nov 27, 2023

robertwb Nov 22, 2023

robertwb Nov 22, 2023

johnjcasey Nov 27, 2023

robertwb Nov 22, 2023

johnjcasey commented Nov 28, 2023

robertwb left a comment

robertwb Dec 5, 2023

robertwb Dec 5, 2023

robertwb Dec 5, 2023

johnjcasey Dec 5, 2023

Feature/dead letter queue core #29164

Feature/dead letter queue core #29164

Conversation

johnjcasey commented Oct 27, 2023

Add the base utilities for creating error handlers and adding DLQs to Java IOs

GitHub Actions Tests Status (on master branch)

johnjcasey commented Nov 16, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnjcasey commented Nov 28, 2023

robertwb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment