Re: [PR] KAFKA-15826: Close consumer when sink task is cancelled [kafka]

2024-01-17 Thread via GitHub
gharris1727 commented on PR #14762: URL: https://github.com/apache/kafka/pull/14762#issuecomment-1896474948 I think due to the locking concerns I raised earlier, and that we can resolve this resource leak in our tests, this PR is not viable to merge. We can revisit this in the future if the

Re: [PR] KAFKA-15826: Close consumer when sink task is cancelled [kafka]

2024-01-17 Thread via GitHub
gharris1727 closed pull request #14762: KAFKA-15826: Close consumer when sink task is cancelled URL: https://github.com/apache/kafka/pull/14762 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the sp

Re: [PR] KAFKA-15826: Close consumer when sink task is cancelled [kafka]

2023-12-11 Thread via GitHub
gharris1727 commented on PR #14762: URL: https://github.com/apache/kafka/pull/14762#issuecomment-1850986158 I found this same leak again when debugging some failures of the `kafka.utils.TestUtils.verifyNoUnexpectedThreads` assertion in core tests. Whenever we leak one of these clients in co

Re: [PR] KAFKA-15826: Close consumer when sink task is cancelled [kafka]

2023-11-17 Thread via GitHub
gharris1727 commented on PR #14762: URL: https://github.com/apache/kafka/pull/14762#issuecomment-1816905171 > What was the conclusion here? Is triggering wakeup on a separate thread (through WorkerSinkTask::stop) before also eventually closing the consumer on a separate thread (through Work

Re: [PR] KAFKA-15826: Close consumer when sink task is cancelled [kafka]

2023-11-17 Thread via GitHub
yashmayya commented on code in PR #14762: URL: https://github.com/apache/kafka/pull/14762#discussion_r1397042412 ## connect/runtime/src/main/java/org/apache/kafka/connect/runtime/WorkerSinkTask.java: ## @@ -163,6 +163,12 @@ public void initialize(TaskConfig taskConfig) {

Re: [PR] KAFKA-15826: Close consumer when sink task is cancelled [kafka]

2023-11-16 Thread via GitHub
gharris1727 commented on PR #14762: URL: https://github.com/apache/kafka/pull/14762#issuecomment-1815495283 Hey @yashmayya Thanks for letting me know the original intent of the test. I agree, changing the test to expect the call to succeed was not in the spirit of the original test, so i've

Re: [PR] KAFKA-15826: Close consumer when sink task is cancelled [kafka]

2023-11-16 Thread via GitHub
yashmayya commented on PR #14762: URL: https://github.com/apache/kafka/pull/14762#issuecomment-1814288871 > Since other workers in the cluster may not have this fix applied, I'll leave the error message unchanged and just update the assertions. Hm, the original purpose of those tests

Re: [PR] KAFKA-15826: Close consumer when sink task is cancelled [kafka]

2023-11-15 Thread via GitHub
gharris1727 commented on PR #14762: URL: https://github.com/apache/kafka/pull/14762#issuecomment-1813403690 The only problem I have with this approach is that the [official documentation](https://kafka.apache.org/36/javadoc/org/apache/kafka/clients/consumer/KafkaConsumer.html#multithreaded)

Re: [PR] KAFKA-15826: Close consumer when sink task is cancelled [kafka]

2023-11-15 Thread via GitHub
gharris1727 commented on PR #14762: URL: https://github.com/apache/kafka/pull/14762#issuecomment-1812953889 @yashmayya I noticed those failures and wasn't sure they're affected, thanks for explaining exactly what is going on! Since other workers in the cluster may not have this fix ap

[PR] KAFKA-15826: Close consumer when sink task is cancelled [kafka]

2023-11-14 Thread via GitHub
gharris1727 opened a new pull request, #14762: URL: https://github.com/apache/kafka/pull/14762 Currently the only place that the consumer is closed is on the task thread, which may be blocked indefinitely by the task plugin. Similar to AbstractWorkerSourceTask, we should close the consumer