lhotari commented on PR #24947: URL: https://github.com/apache/pulsar/pull/24947#issuecomment-3490892390
> Well, there has been a debate before in https://github.com/apache/pulsar/pull/24500/files#r2199584548 > > The timeout failure usually represents a unknown state. Removing it from the cache cannot cancel the ongoing topic loading process in background. There could be two concurrent topic loading tasks if we added the logic, some tests might fail (you can see the changes in #24500). We have to take care of this case. > > Topic loading for non-persistent topics is much more simple than persistent topics, so actually the timeout exception hardly happen. @BewareMyPower Yes, I agree. I came to the same conclusion when looking into the code. However, there's also problems in the current CompletableFuture & timeout based solution. For example, if the topic has already opened the ledger when the timeout occurs, it won't be able to complete the future successfully. The topic needs to be closed after it just got successfully opened: https://github.com/apache/pulsar/blob/39bb67542f2a7b849acaff681d408c693e1a2a18/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java#L1896-L1914 This doesn't make much sense. Instead of using CompletableFuture values in maps, a better approach would be to have some Entry class that holds more related state. There could be a CompletableFuture inside the Entry class. In the topic loading timeout scenario, it doesn't make sense to terminate the topic loading if it's already opening the ledger. The system would just get more loaded due to closing the topic again and reopening it in a retry. -- 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]
