[GitHub] [kafka] vamossagar12 commented on pull request #13594: KAFKA-14913: Using ThreadUtils.shutdownExecutorServiceQuietly to close executors in Connect Runtime
vamossagar12 commented on PR #13594: URL: https://github.com/apache/kafka/pull/13594#issuecomment-1538450687 Thanks @viktorsomogyi . These are the ones we could find so far. We can create follow up tickets if needed for any other executors within connect which need to be closed this way and merge this. WDYT? -- 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 specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [kafka] vamossagar12 commented on pull request #13594: KAFKA-14913: Using ThreadUtils.shutdownExecutorServiceQuietly to close executors in Connect Runtime
vamossagar12 commented on PR #13594: URL: https://github.com/apache/kafka/pull/13594#issuecomment-1537335506 The 2 connect related tests, `testSyncTopicConfigs() – org.apache.kafka.connect.mirror.integration.IdentityReplicationIntegrationTest` and `testGetSinkConnectorOffsetsDifferentKafkaClusterTargeted – org.apache.kafka.connect.integration.OffsetsApiIntegrationTest` seem to be flaky and I have created separate JIRA tickets [here](https://issues.apache.org/jira/browse/KAFKA-14971) and [here](https://issues.apache.org/jira/browse/KAFKA-14956) to track the failures. -- 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 specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [kafka] vamossagar12 commented on pull request #13594: KAFKA-14913: Using ThreadUtils.shutdownExecutorServiceQuietly to close executors in Connect Runtime
vamossagar12 commented on PR #13594: URL: https://github.com/apache/kafka/pull/13594#issuecomment-1529384725 i think this PR. has a few more test errors. I would debug those and push a fix. -- 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 specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [kafka] vamossagar12 commented on pull request #13594: KAFKA-14913: Using ThreadUtils.shutdownExecutorServiceQuietly to close executors in Connect Runtime
vamossagar12 commented on PR #13594: URL: https://github.com/apache/kafka/pull/13594#issuecomment-1527739912 @urbandan , i took the liberty and modified the `shutdownExecutorServiceQuietly` to do a 2-phased shutdown as suggested in JavaDocs. Also, modified `MemoryOffsetBackingStore, SourceTaskOffsetCommitter, Worker` to use `shutdownExecutorServiceQuietly` to shutdown their executors. @yashmayya , as suggested, I have also removed the ConnectException thrown from `MemoryOffsetBackingStore#stop`. This is ready for a re-review. -- 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 specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [kafka] vamossagar12 commented on pull request #13594: KAFKA-14913: Using ThreadUtils.shutdownExecutorServiceQuietly to close executors in Connect Runtime
vamossagar12 commented on PR #13594: URL: https://github.com/apache/kafka/pull/13594#issuecomment-1521506051 I checked the flow again and ConnectException being thrown is handled from `AbstractWorkerSourceTask#close` via the `Utils.closeQuietly` method which the Worker#stop doesn't. Nonetheless I agree throwing ConnectException doesn't make sense (although the resource leaks in Worker can still be solved by invoking Utils.closeQuietly.) Regarding the 2 phase stop for ExceutorService, yeah I think we should do it. @urbandan Since you wrote this method WDYT? We are talking about the 2 phase closure example here: https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/util/concurrent/ExecutorService.html -- 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 specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [kafka] vamossagar12 commented on pull request #13594: KAFKA-14913: Using ThreadUtils.shutdownExecutorServiceQuietly to close executors in Connect Runtime
vamossagar12 commented on PR #13594: URL: https://github.com/apache/kafka/pull/13594#issuecomment-1521193360 @yashmayya , hmm MemoryOffsetBackingStore is used in Connect Standalone IIUC. I am not totally aware of the historical context here of whether throwing a ConnectException is valid or not. Do you reckon changing that behaviour is safe? Regarding the shutdown mechanism in Worker, turns out it is what JavaDocs provides as an example https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/util/concurrent/ExecutorService.html and was pointing out in a code review [here](https://github.com/apache/kafka/pull/11955#discussion_r859425060). Since it's in Worker, I am slightly sceptical in changing this behaviour and also the fact that JavaDocs suggests 2 phase shutdown example. WDYT? -- 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 specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [kafka] vamossagar12 commented on pull request #13594: KAFKA-14913: Using ThreadUtils.shutdownExecutorServiceQuietly to close executors in Connect Runtime
vamossagar12 commented on PR #13594: URL: https://github.com/apache/kafka/pull/13594#issuecomment-1517696428 Thanks @yashmayya . I just tried to include only the ones which have a similar pattern. I think `SourceTaskOffsetCommitter` is something which can also be modified but I missed that. The one in `MemoryOffsetBackingStore` has some handling in cases on InterruptedException and infact throws ConnectException when it can't shutdown in time which is unlike what the newly added utils method does. Specifically on `Worker` I decided not to change because it was added as part of KAFKA-12380. As I said, I changed only those which were similar to the pattern the newly added Utils method had. -- 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 specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [kafka] vamossagar12 commented on pull request #13594: KAFKA-14913: Using ThreadUtils.shutdownExecutorServiceQuietly to close executors in Connect Runtime
vamossagar12 commented on PR #13594: URL: https://github.com/apache/kafka/pull/13594#issuecomment-1514537520 @yashmayya , @C0urante can you also review this small PR whenever you get the chance? Thanlks! -- 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 specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
