KAFKA-16505: Adding dead letter queue in Kafka Streams#17942
KAFKA-16505: Adding dead letter queue in Kafka Streams#17942lucasbru merged 16 commits intoapache:trunkfrom
Conversation
4fc7717 to
5698e60
Compare
| public static final String ENABLE_METRICS_PUSH_DOC = "Whether to enable pushing of internal client metrics for (main, restore, and global) consumers, producers, and admin clients." + | ||
| " The cluster must have a client metrics subscription which corresponds to a client."; | ||
|
|
||
| public static final String ERRORS_DEAD_LETTER_QUEUE_TOPIC_NAME_CONFIG = "errors.dead.letter.queue.topic.name"; |
There was a problem hiding this comment.
Could you please add unit tests?
There was a problem hiding this comment.
You cannot add a class to the public API that was not mentioned in the KIP.
There was a problem hiding this comment.
I renamed this class to ExceptionHandlerUtils, make it packaged protected, and made all methods statics to avoid inheritance.
| public static final String HEADER_ERRORS_OFFSET_NAME = "__streams.errors.offset"; | ||
|
|
||
|
|
||
| public boolean shouldBuildDeadLetterQueueRecord() { |
There was a problem hiding this comment.
Why is this public? It is only used within this class.
There was a problem hiding this comment.
Changed it to package protected
| return this; | ||
| } | ||
|
|
||
| public List<ProducerRecord<byte[], byte[]>> drainDeadLetterQueueRecords() { |
There was a problem hiding this comment.
According to the KIP this should be named deadLetterQueueReords().
There was a problem hiding this comment.
Good point, I renamed it
| /** | ||
| * {@code CommonExceptionHandler} Contains utilities method that could be used by all exception handlers | ||
| */ | ||
| public class CommonExceptionHandler implements Configurable { |
There was a problem hiding this comment.
It would be better to define this class as a immutable utility class in the internal package that provides some utility methods to the default exception handlers. That means, no inheritance.
|
Indeed - using a single collection of dead letter records here that will shared across all stream threads doesn't seem like the right approach to me. Was this intended? I'm not sure if there is a way to replace the enum by a class of some kind where we can properly define a non-static collection of records and remain binary compatible. It seems to me that this needs a new design proposal and an updated KIP? |
|
An option to fix this might be to add a new method |
cadonna
left a comment
There was a problem hiding this comment.
Thanks for the updates @sebastienviale !
I did a first pass. Here my comments.
| return handle(record, exception); | ||
| throw new UnsupportedOperationException(); |
There was a problem hiding this comment.
Shouldn't this still call the other deprecated method?
Imagine a user implemented
handle(final ProducerRecord<byte[], byte[]> record, final Exception exception)but not
ProductionExceptionHandlerResponse handle(final ErrorHandlerContext context,
final ProducerRecord<byte[], byte[]> record,
final Exception exception)Streams would throw an UnsupportedOperationException although it did not before upgrading to this version.
There was a problem hiding this comment.
Maybe it would be beneficial to add some unit tests that verify this redirection. With such unit tests, this removal would had happened without some thoughts about why the test failed.
There was a problem hiding this comment.
indeed, I added a Unit Test that failed if an UnsupportedOperationException is thrown
| /** | ||
| * Represents the result of handling a production exception. | ||
| * <p> | ||
| * The {@code Response} class encapsulates a {@link ProductionExceptionHandlerResponse}, |
There was a problem hiding this comment.
| * The {@code Response} class encapsulates a {@link ProductionExceptionHandlerResponse}, | |
| * The {@code Response} class encapsulates a {@link Result}, |
| * {@link ProducerRecord} instances to be sent to a dead letter queue. | ||
| * </p> | ||
| */ | ||
| class Response { |
There was a problem hiding this comment.
Could you please add some unit tests for this class?
| final ErrorHandlerContext context, | ||
| final Exception e) { | ||
| if (deadLetterQueueTopicName == null) { | ||
| throw new InvalidConfigurationException(String.format("%s can not be null while building DeadLetterQueue record", StreamsConfig.ERRORS_DEAD_LETTER_QUEUE_TOPIC_NAME_CONFIG)); |
There was a problem hiding this comment.
| throw new InvalidConfigurationException(String.format("%s can not be null while building DeadLetterQueue record", StreamsConfig.ERRORS_DEAD_LETTER_QUEUE_TOPIC_NAME_CONFIG)); | |
| throw new InvalidConfigurationException(String.format("%s cannot be null while building dead letter queue record", StreamsConfig.ERRORS_DEAD_LETTER_QUEUE_TOPIC_NAME_CONFIG)); |
| static ProducerRecord<byte[], byte[]> buildDeadLetterQueueRecord(final String deadLetterQueueTopicName, | ||
| final byte[] key, | ||
| final byte[] value, | ||
| final ErrorHandlerContext context, | ||
| final Exception e) { |
There was a problem hiding this comment.
| static ProducerRecord<byte[], byte[]> buildDeadLetterQueueRecord(final String deadLetterQueueTopicName, | |
| final byte[] key, | |
| final byte[] value, | |
| final ErrorHandlerContext context, | |
| final Exception e) { | |
| static ProducerRecord<byte[], byte[]> buildDeadLetterQueueRecord(final String deadLetterQueueTopicName, | |
| final byte[] key, | |
| final byte[] value, | |
| final ErrorHandlerContext context, | |
| final Exception e) { |
| @SuppressWarnings("deprecation") | ||
| @Deprecated | ||
| @Override | ||
| public DeserializationHandlerResponse handle(final ErrorHandlerContext context, |
There was a problem hiding this comment.
You can remove those deprecated handler methods. They are not called anywhere in the Streams code.
| @Deprecated | ||
| @Override | ||
| public ProcessingHandlerResponse handle(final ErrorHandlerContext context, final Record<?, ?> record, final Exception exception) { | ||
| public ProcessingHandlerResponse handle(final ErrorHandlerContext context, |
There was a problem hiding this comment.
You can remove this deprecated handler method. It is not called anywhere in the Streams code.
| public Response handleError(final ErrorHandlerContext context, | ||
| final ConsumerRecord<byte[], byte[]> record, | ||
| final Exception exception) { | ||
| log.warn( |
There was a problem hiding this comment.
This should be log.error(). It is failing, not resuming.
| @SuppressWarnings("deprecation") | ||
| @Deprecated | ||
| @Override | ||
| public DeserializationHandlerResponse handle(final ErrorHandlerContext context, |
There was a problem hiding this comment.
You can remove those deprecated handler methods. They are not called anywhere in the Streams code.
| public ProcessingHandlerResponse handle(final ErrorHandlerContext context, | ||
| final Record<?, ?> record, final Exception exception) { |
There was a problem hiding this comment.
You can remove this deprecated handler method. It is not called anywhere in the Streams code.
93cd94d to
93c831f
Compare
@cadonna thanks for your review, I pushed changes |
fc4ac3c to
efa1cae
Compare
efa1cae to
739f443
Compare
This PR is part of the KIP-1034. It brings the support for the source raw key and the source raw value in the `ErrorHandlerContext`. Required by the routing to DLQ implemented by #17942. Reviewers: Bruno Cadonna <cadonna@apache.org> Co-authored-by: Damien Gasparina <d.gasparina@gmail.com>
22a8d80 to
2c32c96
Compare
This PR is part of the KIP-1034. It brings the support for the source raw key and the source raw value in the `ErrorHandlerContext`. Required by the routing to DLQ implemented by apache#17942. Reviewers: Bruno Cadonna <cadonna@apache.org> Co-authored-by: Damien Gasparina <d.gasparina@gmail.com>
There was a problem hiding this comment.
Pull Request Overview
Adds support for Dead Letter Queue (DLQ) in Kafka Streams by extending exception handler APIs and wiring DLQ record production throughout the core processing, deserialization, and production paths.
- Introduced a new
ResponseAPI (withdeadLetterQueueRecords) for deserialization, processing, and production exception handlers. - Updated core classes (
StreamTask,RecordDeserializer,RecordCollector,ProcessorNode) to emit DLQ records when configured. - Added
ExceptionHandlerUtilsfor building DLQ records with appropriate headers and comprehensive tests validating DLQ behavior.
Reviewed Changes
Copilot reviewed 24 out of 24 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| streams/src/test/java/org/apache/kafka/test/MockRecordCollector.java | Added overload of send to collect serialized records for DLQ. |
| streams/src/test/java/org/apache/kafka/streams/processor/internals/StreamTaskTest.java | Renamed handler methods, switched to Response type. |
| streams/src/test/java/org/apache/kafka/streams/processor/internals/RecordDeserializerTest.java | Switched to new Response API and added DLQ tests. |
| streams/src/test/java/org/apache/kafka/streams/processor/internals/RecordCollectorTest.java | Switched to new Response API and added DLQ tests. |
| streams/src/test/java/org/apache/kafka/streams/processor/internals/ProcessorNodeTest.java | Switched to new Response API and added DLQ tests. |
| streams/src/test/java/org/apache/kafka/streams/errors/ExceptionHandlerUtilsTest.java | New tests for DLQ record headers. |
| streams/src/test/java/org/apache/kafka/streams/StreamsConfigTest.java | Added test for default DLQ config. |
| streams/src/main/java/org/apache/kafka/streams/processor/internals/StreamTask.java | Integrated DLQ sends into processing error flow. |
| streams/src/main/java/org/apache/kafka/streams/processor/internals/RecordDeserializer.java | Integrated DLQ sends into deserialization error flow. |
| streams/src/main/java/org/apache/kafka/streams/processor/internals/RecordCollectorImpl.java | Integrated DLQ sends into production and serialization flows. |
| streams/src/main/java/org/apache/kafka/streams/processor/internals/RecordCollector.java | Added new send signature for DLQ. |
| streams/src/main/java/org/apache/kafka/streams/processor/internals/ProcessorNode.java | Integrated DLQ sends into processing node error flow. |
| streams/src/main/java/org/apache/kafka/streams/errors/internals/ExceptionHandlerUtils.java | New utility for building DLQ records with headers. |
| streams/src/main/java/org/apache/kafka/streams/errors/ProductionExceptionHandler.java | Introduced Response, deprecated old API, added DLQ support. |
| streams/src/main/java/org/apache/kafka/streams/errors/ProcessingExceptionHandler.java | Introduced Response, deprecated old API, added DLQ support. |
| streams/src/main/java/org/apache/kafka/streams/errors/LogAndFailProcessingExceptionHandler.java | Added DLQ topic configuration and records. |
| streams/src/main/java/org/apache/kafka/streams/errors/LogAndContinueProcessingExceptionHandler.java | Added DLQ topic configuration and records. |
| streams/src/main/java/org/apache/kafka/streams/errors/LogAndFailExceptionHandler.java | Added DLQ topic configuration and records. |
| streams/src/main/java/org/apache/kafka/streams/errors/LogAndContinueExceptionHandler.java | Added DLQ topic configuration and records. |
| streams/src/main/java/org/apache/kafka/streams/errors/DefaultProductionExceptionHandler.java | Added DLQ topic configuration and records. |
| streams/src/main/java/org/apache/kafka/streams/StreamsConfig.java | Defined new errors.dead.letter.queue.topic.name config. |
| streams/integration-tests/src/test/java/org/apache/kafka/streams/integration/SwallowUnknownTopicErrorIntegrationTest.java | Switched to new Response API. |
| streams/integration-tests/src/test/java/org/apache/kafka/streams/integration/ProcessingExceptionHandlerIntegrationTest.java | Switched to new Response API. |
Comments suppressed due to low confidence (1)
streams/src/test/java/org/apache/kafka/streams/StreamsConfigTest.java:1619
- This test method is missing the @test annotation, so it will not be executed by the test runner.
public void shouldSetDefaultDeadLetterQueue() {
lucasbru
left a comment
There was a problem hiding this comment.
Thanks for the PR! I left a few questions for my own understanding, and a few nits. Mostly looking good to me, though!
# Conflicts: # streams/src/main/java/org/apache/kafka/streams/processor/internals/RecordCollectorImpl.java
c1c6eea to
f1b8286
Compare
lucasbru
left a comment
There was a problem hiding this comment.
LGTM!
@cadonna would you like to take another look?
@Dabz @sebastienviale I wonder if we could have a little integration tests for this feature, to test that it actually works end-to-end?
|
I restarted the CI since it was triggering a flaky test. I created https://issues.apache.org/jira/browse/KAFKA-16505 for tracking the integration test. |
Implements KIP-1034 to add support of Dead Letter Queue in Kafka Streams. Reviewers: Lucas Brutschy <lbrutschy@confluent.io>, Bruno Cadonna <cadonna@apache.org> Co-authored-by: Sebastien Viale <sebastien.viale@michelin.com>
| e.printStackTrace(stackTracePrintWriter); | ||
|
|
||
| try (final StringSerializer stringSerializer = new StringSerializer()) { | ||
| producerRecord.headers().add(HEADER_ERRORS_EXCEPTION_NAME, stringSerializer.serialize(null, e.toString())); |
There was a problem hiding this comment.
KIP-1034 states that this filed is for the "Name of the thrown exception", so we should use e.getClass().getName() instead, right?
There was a problem hiding this comment.
Also, shouldn't we include the headers from the origin record in the DLQ record? I noticed KIP-1034 states "Existing context headers are automatically forwarded into the new DLQ record"
There was a problem hiding this comment.
Yes, you are right there is a mismatch between the KIP and the implementation here.
…21370) This patch fixed two mismatch between [KIP-1034](https://cwiki.apache.org/confluence/display/KAFKA/KIP-1034%3A+Dead+letter+queue+in+Kafka+Streams#KIP1034:DeadletterqueueinKafkaStreams-ProposedChanges) and the actual implementation: - Incorrect value of `HEADER_ERRORS_EXCEPTION_NAME`, it should be the name of the thrown exception, not exception's String representation. - Original headers from record that causes exception should be added to dlq record headers. References: - #17942 (comment) - #17942 (comment) - [KIP-1034](https://cwiki.apache.org/confluence/display/KAFKA/KIP-1034%3A+Dead+letter+queue+in+Kafka+Streams#KIP1034:DeadletterqueueinKafkaStreams-ProposedChanges) Reviewers: Lucas Brutschy <lbrutschy@confluent.io>
…pache#21370) This patch fixed two mismatch between [KIP-1034](https://cwiki.apache.org/confluence/display/KAFKA/KIP-1034%3A+Dead+letter+queue+in+Kafka+Streams#KIP1034:DeadletterqueueinKafkaStreams-ProposedChanges) and the actual implementation: - Incorrect value of `HEADER_ERRORS_EXCEPTION_NAME`, it should be the name of the thrown exception, not exception's String representation. - Original headers from record that causes exception should be added to dlq record headers. References: - apache#17942 (comment) - apache#17942 (comment) - [KIP-1034](https://cwiki.apache.org/confluence/display/KAFKA/KIP-1034%3A+Dead+letter+queue+in+Kafka+Streams#KIP1034:DeadletterqueueinKafkaStreams-ProposedChanges) Reviewers: Lucas Brutschy <lbrutschy@confluent.io>
…pache#21370) This patch fixed two mismatch between [KIP-1034](https://cwiki.apache.org/confluence/display/KAFKA/KIP-1034%3A+Dead+letter+queue+in+Kafka+Streams#KIP1034:DeadletterqueueinKafkaStreams-ProposedChanges) and the actual implementation: - Incorrect value of `HEADER_ERRORS_EXCEPTION_NAME`, it should be the name of the thrown exception, not exception's String representation. - Original headers from record that causes exception should be added to dlq record headers. References: - apache#17942 (comment) - apache#17942 (comment) - [KIP-1034](https://cwiki.apache.org/confluence/display/KAFKA/KIP-1034%3A+Dead+letter+queue+in+Kafka+Streams#KIP1034:DeadletterqueueinKafkaStreams-ProposedChanges) Reviewers: Lucas Brutschy <lbrutschy@confluent.io>
…21378) This is a backport of PR #21370, with an additional checkstyle fix (adding a `final` keyword to make checkstyle works in 4.2 branch). --- This patch fixed two mismatch between [KIP-1034](https://cwiki.apache.org/confluence/display/KAFKA/KIP-1034%3A+Dead+letter+queue+in+Kafka+Streams#KIP1034:DeadletterqueueinKafkaStreams-ProposedChanges) and the actual implementation: - Incorrect value of `HEADER_ERRORS_EXCEPTION_NAME`, it should be the name of the thrown exception, not exception's String representation. - Original headers from record that causes exception should be added to dlq record headers. References: - #17942 (comment) - #17942 (comment) - [KIP-1034](https://cwiki.apache.org/confluence/display/KAFKA/KIP-1034%3A+Dead+letter+queue+in+Kafka+Streams#KIP1034:DeadletterqueueinKafkaStreams-ProposedChanges) Reviewers: Lucas Brutschy <lbrutschy@confluent.io>, Chia-Ping Tsai <chia7712@gmail.com>
First Pull Request to implement KIP-1034 to add support of Dead Letter
Queue in Kafka Streams. A second PR will store the source record key and
value byte[] in the context to add it to the DLQ records.
Reviewers: Lucas Brutschy lbrutschy@confluent.io, Bruno Cadonna
cadonna@apache.org