-
Notifications
You must be signed in to change notification settings - Fork 15.4k
KAFKA-13348: Allow Source Tasks to Handle Producer Exceptions #11382
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 3 commits
34d93a8
b956c19
22daf36
878b131
ad098eb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -39,6 +39,7 @@ | |
| import org.apache.kafka.connect.runtime.distributed.ClusterConfigState; | ||
| import org.apache.kafka.connect.runtime.errors.RetryWithToleranceOperator; | ||
| import org.apache.kafka.connect.runtime.errors.Stage; | ||
| import org.apache.kafka.connect.runtime.errors.ToleranceType; | ||
| import org.apache.kafka.connect.source.SourceRecord; | ||
| import org.apache.kafka.connect.source.SourceTask; | ||
| import org.apache.kafka.connect.storage.CloseableOffsetStorageReader; | ||
|
|
@@ -364,9 +365,16 @@ private boolean sendRecords() { | |
| producerRecord, | ||
| (recordMetadata, e) -> { | ||
| if (e != null) { | ||
| log.error("{} failed to send record to {}: ", WorkerSourceTask.this, topic, e); | ||
| log.trace("{} Failed record: {}", WorkerSourceTask.this, preTransformRecord); | ||
| producerSendException.compareAndSet(null, e); | ||
| if (retryWithToleranceOperator.getErrorToleranceType().equals(ToleranceType.ALL)) { | ||
| // executeFailed here allows the use of existing logging infrastructure/configuration | ||
| retryWithToleranceOperator.executeFailed(Stage.KAFKA_PRODUCE, WorkerSourceTask.class, | ||
| preTransformRecord, e); | ||
| commitTaskRecord(preTransformRecord, null); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we have a debug/trace log in this path?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Previously it was suggested to have the tolerance operator handle via the logging report. I would personally find it useful to have it in the connect log regardless of tolerance error logging configuration. I've moved the error/debug log lines to above the tolerance check to log in all instances.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should not be logging at
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's keep the existing
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My misunderstanding, thank you both for the feedback. Update made. |
||
| } else { | ||
| log.error("{} failed to send record to {}: ", WorkerSourceTask.this, topic, e); | ||
| log.trace("{} Failed record: {}", WorkerSourceTask.this, preTransformRecord); | ||
| producerSendException.compareAndSet(null, e); | ||
| } | ||
| } else { | ||
| submittedRecord.ack(); | ||
| counter.completeRecord(); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -111,6 +111,23 @@ public synchronized Future<Void> executeFailed(Stage stage, Class<?> executingCl | |
| return errantRecordFuture; | ||
| } | ||
|
|
||
| public synchronized Future<Void> executeFailed(Stage stage, Class<?> executingClass, | ||
| SourceRecord sourceRecord, | ||
| Throwable error) { | ||
|
|
||
| markAsFailed(); | ||
| context.sourceRecord(sourceRecord); | ||
| context.currentContext(stage, executingClass); | ||
| context.error(error); | ||
| errorHandlingMetrics.recordFailure(); | ||
| Future<Void> errantRecordFuture = context.report(); | ||
| if (!withinToleranceLimits()) { | ||
| errorHandlingMetrics.recordError(); | ||
| throw new ConnectException("Tolerance exceeded in error handler", error); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Now that this message can come from 2 different paths, should we add some context to the message to disambiguate them?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added some context to the string error message denoting it was a Source Worker. I am open to suggestions on how verbose this message should be. |
||
| } | ||
| return errantRecordFuture; | ||
| } | ||
|
|
||
| /** | ||
| * Execute the recoverable operation. If the operation is already in a failed state, then simply return | ||
| * with the existing failure. | ||
|
|
@@ -229,6 +246,13 @@ public synchronized boolean withinToleranceLimits() { | |
| } | ||
| } | ||
|
|
||
| // For source connectors that want to skip kafka producer errors. | ||
| // They cannot use withinToleranceLimits() as no failure may have actually occurred prior to the producer failing | ||
| // to write to kafka. | ||
| public synchronized ToleranceType getErrorToleranceType() { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this need to be
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It does not. Type is immutable and thread safe. I had dug through the ticket that retroactively made this class thread safe and it seemed like a good idea at the time to slap a synchronized on it to match the rest of the class, but is not necessary at all. Removed. |
||
| return errorToleranceType; | ||
| } | ||
|
|
||
| // Visible for testing | ||
| boolean checkRetry(long startTime) { | ||
| return (time.milliseconds() - startTime) < errorRetryTimeout; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -222,6 +222,13 @@ private void createWorkerTask() { | |
| createWorkerTask(TargetState.STARTED); | ||
| } | ||
|
|
||
| private void createWorkerTaskWithErrorToleration() { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we reuse the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1 I have refactored the constructors to be cleaner with various parameter lists. |
||
| workerTask = new WorkerSourceTask(taskId, sourceTask, statusListener, TargetState.STARTED, keyConverter, valueConverter, headerConverter, | ||
| transformationChain, producer, admin, TopicCreationGroup.configuredGroups(sourceConfig), | ||
| offsetReader, offsetWriter, config, clusterConfigState, metrics, plugins.delegatingLoader(), Time.SYSTEM, | ||
| RetryWithToleranceOperatorTest.ALL_OPERATOR, statusBackingStore, Runnable::run); | ||
| } | ||
|
|
||
| private void createWorkerTask(TargetState initialState) { | ||
| createWorkerTask(initialState, keyConverter, valueConverter, headerConverter); | ||
| } | ||
|
|
@@ -815,6 +822,32 @@ public void testSendRecordsTaskCommitRecordFail() throws Exception { | |
| PowerMock.verifyAll(); | ||
| } | ||
|
|
||
| @Test | ||
| public void testSourceTaskIgnoresProducerException() throws Exception { | ||
| createWorkerTaskWithErrorToleration(); | ||
| expectTopicCreation(TOPIC); | ||
|
|
||
| // send two records | ||
| // record 1 will succeed | ||
| // record 2 will invoke the producer's failure callback, but ignore the exception via retryOperator | ||
| // and no ConnectException will be thrown | ||
| SourceRecord record1 = new SourceRecord(PARTITION, OFFSET, TOPIC, 1, KEY_SCHEMA, KEY, RECORD_SCHEMA, RECORD); | ||
| SourceRecord record2 = new SourceRecord(PARTITION, OFFSET, TOPIC, 2, KEY_SCHEMA, KEY, RECORD_SCHEMA, RECORD); | ||
|
|
||
|
|
||
| expectSendRecordOnce(); | ||
| expectSendRecordProducerCallbackFail(); | ||
| sourceTask.commitRecord(EasyMock.anyObject(SourceRecord.class), EasyMock.anyObject(RecordMetadata.class)); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1 |
||
| EasyMock.expectLastCall(); | ||
|
|
||
| PowerMock.replayAll(); | ||
|
|
||
| Whitebox.setInternalState(workerTask, "toSend", Arrays.asList(record1, record2)); | ||
| Whitebox.invokeMethod(workerTask, "sendRecords"); | ||
|
|
||
| PowerMock.verifyAll(); | ||
| } | ||
|
|
||
| @Test | ||
| public void testSlowTaskStart() throws Exception { | ||
| final CountDownLatch startupLatch = new CountDownLatch(1); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can use
==to compare enums.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1