KAFKA-9830: Implement AutoCloseable in ErrorReporter and subclasses by gharris1727 · Pull Request #8442 · apache/kafka

gharris1727 · 2020-04-08T00:17:59Z

The DeadLetterQueueReporter has a KafkaProducer that it must close to clean up resources
Currently, the producer and its threads are leaked every time a task is stopped
Responsibility for cleaning up ErrorReporters is transitively assigned to the
ProcessingContext, RetryWithToleranceOperator, and WorkerSinkTask/WorkerSinkTask classes
One new unit test in ErrorReporterTest asserts that the producer is closed by the dlq reporter

Signed-off-by: Greg Harris gregh@confluent.io

More detailed description of your change,
if necessary. The PR title and PR message become
the squashed commit message, so use a separate
comment to ping reviewers.

Summary of testing strategy (including rationale)
for the feature or bug fix. Unit and/or integration
tests are expected for any behaviour change and
system tests should be considered for larger changes.

Committer Checklist (excluded from commit message)

Verify design and implementation
Verify test coverage and CI build status
Verify documentation (including upgrade notes)

* The DeadLetterQueueReporter has a KafkaProducer that it must close to clean up resources * Currently, the producer and it's threads are leaked every time a task is stopped * Responsibility for cleaning up ErrorReporters is transitively assigned to the ProcessingContext, RetryWithToleranceOperator, and WorkerSinkTask/WorkerSinkTask classes * One new unit test in ErrorReporterTest asserts that the producer is closed by the dlq reporter Signed-off-by: Greg Harris <gregh@confluent.io>

gharris1727 · 2020-04-08T00:18:42Z

cc @wicknicks @C0urante @ncliang for review

wicknicks

thanks for the quick fix, @gharris1727. one nit.

…atibility Signed-off-by: Greg Harris <gregh@confluent.io>

… task stop. Signed-off-by: Greg Harris <gregh@confluent.io>

gharris1727 · 2020-04-08T01:20:58Z

Thanks for your comments @wicknicks @C0urante, this is ready for a second pass.

C0urante

Still have a slight preference against adding a default implementation for ErrorReporter::close but will leave that to greater minds. LGTM, nice find @gharris1727!

wicknicks

Thanks! LGTM!

kkonstantine · 2020-04-08T03:57:19Z

ok to test

…rown. Signed-off-by: Greg Harris <gregh@confluent.io>

… exceptions Also add a test to verify that multiple reporters are closed despite exceptions Signed-off-by: Greg Harris <gregh@confluent.io>

gharris1727 · 2020-04-08T16:48:29Z

@kkonstantine It appears that the tests failed because of some unrelated checkstyle issue on trunk that has been fixed, can you re-run the tests?

kkonstantine · 2020-04-29T03:00:59Z

ok to test

kkonstantine · 2020-04-29T03:01:37Z

retest this please

kkonstantine

Thanks @gharris1727 !
Important not to leak producers when tasks close

I have a couple suggestions to make this more concise and a recommendation to avoid instantiating an exception if it's not going to be used.

Signed-off-by: Greg Harris <gregh@confluent.io>

gharris1727 · 2020-04-29T19:37:28Z

@kkonstantine I've implemented your feedback and this is ready for another pass.

kkonstantine · 2020-04-29T21:15:13Z

retest this please

kkonstantine

jdk14 green, jdk8 2 failures in streams tests, jdk11 4 failures, also unrelated.

LGTM!

…8442) * The DeadLetterQueueReporter has a KafkaProducer that it must close to clean up resources * Currently, the producer and its threads are leaked every time a task is stopped * Responsibility for cleaning up ErrorReporters is transitively assigned to the ProcessingContext, RetryWithToleranceOperator, and WorkerSinkTask/WorkerSinkTask classes * One new unit test in ErrorReporterTest asserts that the producer is closed by the dlq reporter Reviewers: Arjun Satish <arjun@confluent.io>, Chris Egerton <chrise@confluent.io>, Chia-Ping Tsai <chia7712@gmail.com>, Konstantine Karantasis <konstantine@confluent.io>

…/master` * apache-github/trunk: (45 commits) MINOR: Fix broken JMX link in docs by adding missing starting double quote (apache#8587) KAFKA-9652: Fix throttle metric in RequestChannel and request log due to KIP-219 (apache#8567) KAFKA-9922: Update demo instructions in examples README (apache#8559) KAFKA-9830: Implement AutoCloseable in ErrorReporter and subclasses (apache#8442) KAFKA-9875: Make integration tests more resilient (apache#8578) KAFKA-9932: Don't load configs from ZK when the log has already been loaded (apache#8582) KAFKA-9925: decorate pseudo-topics with app id (apache#8574) KAFKA-9832: fix attempt to commit non-running tasks (apache#8580) KAFKA-9127: don't create StreamThreads for global-only topology (apache#8540) MINOR: add support for kafka 2.4 and 2.5 to downgrade test KAFKA-9176: Retry on getting local stores from KafkaStreams (apache#8568) KAFKA-9823: Follow-up, check state for handling commit error response (apache#8548) KAFKA-6145: KIP-441: Add TaskAssignor class config (apache#8541) MINOR: Fix partition numbering from 0 to P-1 instead of P in docs (apache#8572) KAFKA-9921: disable caching on stores configured to retain duplicates (apache#8564) Minor: remove redundant check in auto preferred leader election (apache#8566) MINOR: Update the link to the Raft paper in docs (apache#8560) MINOR: Fix typos in config properties in MM2 test (apache#8561) MINOR: Improve producer test BufferPoolTest#testCloseNotifyWaiters. (apache#7982) MINOR: document how to escape json parameters to ducktape tests (apache#8546) ...

There was a minor conflict in gradle.properties because the default Scala version changed upstream to Scala 2.13. I kept the upstream change. Related to this, I have updated Jenkinsfile to compile and validate with Scala 2.12 in a separate stage so that we ensure we maintain compatibility. Unlike Apache Kafka, we only run the tests with the default Scala version, which is now 2.13. * apache-github/trunk: (45 commits) MINOR: Fix broken JMX link in docs by adding missing starting double quote (apache#8587) KAFKA-9652: Fix throttle metric in RequestChannel and request log due to KIP-219 (apache#8567) KAFKA-9922: Update demo instructions in examples README (apache#8559) KAFKA-9830: Implement AutoCloseable in ErrorReporter and subclasses (apache#8442) KAFKA-9875: Make integration tests more resilient (apache#8578) KAFKA-9932: Don't load configs from ZK when the log has already been loaded (apache#8582) KAFKA-9925: decorate pseudo-topics with app id (apache#8574) KAFKA-9832: fix attempt to commit non-running tasks (apache#8580) KAFKA-9127: don't create StreamThreads for global-only topology (apache#8540) MINOR: add support for kafka 2.4 and 2.5 to downgrade test KAFKA-9176: Retry on getting local stores from KafkaStreams (apache#8568) KAFKA-9823: Follow-up, check state for handling commit error response (apache#8548) KAFKA-6145: KIP-441: Add TaskAssignor class config (apache#8541) MINOR: Fix partition numbering from 0 to P-1 instead of P in docs (apache#8572) KAFKA-9921: disable caching on stores configured to retain duplicates (apache#8564) Minor: remove redundant check in auto preferred leader election (apache#8566) MINOR: Update the link to the Raft paper in docs (apache#8560) MINOR: Fix typos in config properties in MM2 test (apache#8561) MINOR: Improve producer test BufferPoolTest#testCloseNotifyWaiters. (apache#7982) MINOR: document how to escape json parameters to ducktape tests (apache#8546) ...

…pache#8442) * The DeadLetterQueueReporter has a KafkaProducer that it must close to clean up resources * Currently, the producer and its threads are leaked every time a task is stopped * Responsibility for cleaning up ErrorReporters is transitively assigned to the ProcessingContext, RetryWithToleranceOperator, and WorkerSinkTask/WorkerSinkTask classes * One new unit test in ErrorReporterTest asserts that the producer is closed by the dlq reporter Reviewers: Arjun Satish <arjun@confluent.io>, Chris Egerton <chrise@confluent.io>, Chia-Ping Tsai <chia7712@gmail.com>, Konstantine Karantasis <konstantine@confluent.io>

…8442) * The DeadLetterQueueReporter has a KafkaProducer that it must close to clean up resources * Currently, the producer and its threads are leaked every time a task is stopped * Responsibility for cleaning up ErrorReporters is transitively assigned to the ProcessingContext, RetryWithToleranceOperator, and WorkerSinkTask/WorkerSinkTask classes * One new unit test in ErrorReporterTest asserts that the producer is closed by the dlq reporter Reviewers: Arjun Satish <arjun@confluent.io>, Chris Egerton <chrise@confluent.io>, Chia-Ping Tsai <chia7712@gmail.com>, Konstantine Karantasis <konstantine@confluent.io>

…pache#8442) * The DeadLetterQueueReporter has a KafkaProducer that it must close to clean up resources * Currently, the producer and its threads are leaked every time a task is stopped * Responsibility for cleaning up ErrorReporters is transitively assigned to the ProcessingContext, RetryWithToleranceOperator, and WorkerSinkTask/WorkerSinkTask classes * One new unit test in ErrorReporterTest asserts that the producer is closed by the dlq reporter Reviewers: Arjun Satish <arjun@confluent.io>, Chris Egerton <chrise@confluent.io>, Chia-Ping Tsai <chia7712@gmail.com>, Konstantine Karantasis <konstantine@confluent.io>

wicknicks reviewed Apr 8, 2020

View reviewed changes

Comment thread connect/runtime/src/main/java/org/apache/kafka/connect/runtime/errors/LogReporter.java Outdated

Make no-op close a default method in ErrorReporter for backwards-comp…

60bd1d4

…atibility Signed-off-by: Greg Harris <gregh@confluent.io>

C0urante reviewed Apr 8, 2020

View reviewed changes

Comment thread connect/runtime/src/test/java/org/apache/kafka/connect/runtime/errors/ErrorReporterTest.java

Add test that WorkerSourceTask/WorkerSinkTask close ErrorReporters on…

9973816

… task stop. Signed-off-by: Greg Harris <gregh@confluent.io>

C0urante approved these changes Apr 8, 2020

View reviewed changes

wicknicks approved these changes Apr 8, 2020

View reviewed changes

kkonstantine added the connect label Apr 8, 2020

kkonstantine marked this pull request as ready for review April 8, 2020 03:57

chia7712 reviewed Apr 8, 2020

View reviewed changes

Comment thread connect/runtime/src/main/java/org/apache/kafka/connect/runtime/errors/ProcessingContext.java Outdated

chia7712 reviewed Apr 8, 2020

View reviewed changes

Comment thread connect/runtime/src/main/java/org/apache/kafka/connect/runtime/errors/ProcessingContext.java Outdated

gharris1727 added 2 commits April 8, 2020 09:19

Remove throws from new close methods where no checked exception is th…

5173af7

…rown. Signed-off-by: Greg Harris <gregh@confluent.io>

Handle exceptions from closing each reporter to collect as suppressed…

a23c456

… exceptions Also add a test to verify that multiple reporters are closed despite exceptions Signed-off-by: Greg Harris <gregh@confluent.io>

kkonstantine reviewed Apr 29, 2020

View reviewed changes

Use Utils.closeQuietly and defer creation of exception in loop

a101cd7

Signed-off-by: Greg Harris <gregh@confluent.io>

kkonstantine approved these changes Apr 30, 2020

View reviewed changes

kkonstantine merged commit 0ee19e8 into apache:trunk Apr 30, 2020

gharris1727 deleted the dlq-leak-producer branch April 30, 2020 21:36

gharris1727 mentioned this pull request Apr 30, 2020

KAFKA-9830: Backport "Implement AutoCloseable in ErrorReporter and subclasses" #8594

Closed

3 tasks

C0urante mentioned this pull request Nov 30, 2021

KAFKA-12965 - Graceful clean up of task error metrics #10910

Merged

Conversation

gharris1727 commented Apr 8, 2020 • edited by kkonstantine Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Committer Checklist (excluded from commit message)

Uh oh!

gharris1727 commented Apr 8, 2020

Uh oh!

wicknicks left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

gharris1727 commented Apr 8, 2020

Uh oh!

C0urante left a comment

Choose a reason for hiding this comment

Uh oh!

wicknicks left a comment

Choose a reason for hiding this comment

Uh oh!

kkonstantine commented Apr 8, 2020

Uh oh!

Uh oh!

Uh oh!

gharris1727 commented Apr 8, 2020

Uh oh!

kkonstantine commented Apr 29, 2020

Uh oh!

kkonstantine commented Apr 29, 2020

Uh oh!

kkonstantine left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gharris1727 commented Apr 29, 2020

Uh oh!

kkonstantine commented Apr 29, 2020

Uh oh!

kkonstantine left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

gharris1727 commented Apr 8, 2020 •

edited by kkonstantine

Loading