oneby-wang commented on PR #24698: URL: https://github.com/apache/pulsar/pull/24698#issuecomment-3251455190
Hi, @lhotari thanks for your code review and suggestion, I didn't go that far before, I encountered this problem when I contributed to spring-pulsar, see [DefaultPulsarConsumerFactoryTests.java](https://github.com/spring-projects/spring-pulsar/pull/1242/files#diff-0829bf0b489931cb5dd4714b78464dce81141d848a940f63fcc0b598fcddab92). First, ignore errors or not? I think throwing Exception is very important(for debug reason), unless error messages already printed. So it is safe to ignore the following two errors and continue `CompletableFuture` progress. 1. `watcherFuture` error: already printed. https://github.com/apache/pulsar/blob/2e6a8eb1f200ca687064058a101963713b896fe2/pulsar-client/src/main/java/org/apache/pulsar/client/impl/TopicListWatcher.java#L94-L102 and https://github.com/apache/pulsar/blob/2e6a8eb1f200ca687064058a101963713b896fe2/pulsar-client/src/main/java/org/apache/pulsar/client/impl/TopicListWatcher.java#L164-L177 2. `runningTaskCancelCloseFuture` error: already ignored. https://github.com/apache/pulsar/blob/2e6a8eb1f200ca687064058a101963713b896fe2/pulsar-client/src/main/java/org/apache/pulsar/client/impl/PatternConsumerUpdateQueue.java#L232-L242 Second, close execution order. `topicListWatcherCloseFuture` and `runningTaskCancelCloseFuture` be closed concurrently, after they are closed, then close `super.closeAsync()`. But one thing, add `volatile` on `PatternConsumerUpdateQueue.closed` field, because `PatternMultiTopicsConsumerImpl.closeAsync()` method and `PatternConsumerUpdateQueue.triggerNextTask()` method may probably invoked in different threads. https://github.com/apache/pulsar/blob/2e6a8eb1f200ca687064058a101963713b896fe2/pulsar-client/src/main/java/org/apache/pulsar/client/impl/PatternConsumerUpdateQueue.java#L76 `TopicListWatcher` will append TopicsRemovedOp and TopicsAddedOp to `patternConsumerUpdateQueue`, which will call `PatternConsumerUpdateQueue.triggerNextTask()` method. https://github.com/apache/pulsar/blob/2e6a8eb1f200ca687064058a101963713b896fe2/pulsar-client/src/main/java/org/apache/pulsar/client/impl/TopicListWatcher.java#L280-L283 Undoubtedly, `super.closeAsync()`(close multi-consumers) should be closed after all PatternMultiTopicsConsumerImpl tasks are closed. As you suggested and my replied content, I applied the code review change and pushed to this pull request. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
