Re: [PR] [improve][broker] Optimize Reader creation in TopicPoliciesService [pulsar]

[via GitHub]

BewareMyPower commented on code in PR #24658:
URL: https://github.com/apache/pulsar/pull/24658#discussion_r2296722735


##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/SystemTopicBasedTopicPoliciesService.java:
##########
@@ -473,15 +478,16 @@ public boolean test(NamespaceBundle namespaceBundle) {
     private void initPolicesCache(SystemTopicClient.Reader<PulsarEvent> 
reader, CompletableFuture<Void> future) {
         if (closed.get()) {
             future.completeExceptionally(new 
BrokerServiceException(getClass().getName() + " is closed."));
-            
cleanCacheAndCloseReader(reader.getSystemTopic().getTopicName().getNamespaceObject(),
 false);
+            
cleanCache(reader.getSystemTopic().getTopicName().getNamespaceObject(), false, 
true);
             return;
         }
         reader.hasMoreEventsAsync().whenComplete((hasMore, ex) -> {
             if (ex != null) {
                 log.error("[{}] Failed to check the move events for the system 
topic",
                         reader.getSystemTopic().getTopicName(), ex);
                 future.completeExceptionally(ex);
-                
cleanCacheAndCloseReader(reader.getSystemTopic().getTopicName().getNamespaceObject(),
 false);
+                
cleanCache(reader.getSystemTopic().getTopicName().getNamespaceObject(), false,
+                        isAlreadyClosedException(ex));

Review Comment:
   BTW, could you address the comment here 
https://github.com/apache/pulsar/pull/24658?notification_referrer_id=NT_kwDOARXIg7QxODM5MzEyMjI2ODoxODIwNDgwMw&notifications_query=is%3Aunread#pullrequestreview-3149250672
 as well? `cleanCache(namespace, true, true, true);` is really hard to read, 
just like the `close` method of `PersistentTopic`.
   
   I admit IDE could show the parameter names, but it's still not clear to know 
what the boolean argument represent. For example, in my previous comment 
https://github.com/apache/pulsar/pull/24658?notification_referrer_id=NT_kwDOARXIg7QxODM5MzEyMjI2ODoxODIwNDgwMw&notifications_query=is%3Aunread#discussion_r2296574279,
 it surprised me that it's reasonable to skip clearing `policyCacheInitMap` if 
we're going to skip clearing `readerCache`. It would be more readable to add a 
separated method to do that rather than using a boolean flag to control the 
behavior.
   
   The existing code looks like:
   
   ```java
   void cleanup(String key, boolean closeA, boolean closeB) {
       if (closeA()) {
           doCloseA();
       }
       if (closeB) {
           doCloseB();
           return; // it looks like a wrong logic, but it's correct because B 
and C are coupled, which is implicit
       }
       closeC();
   }
   ```
   
   And it can be called like `cleanup(key, true, true)`, `cleanup(key, true, 
false)`, `cleanup(key, false, true)`, `cleanup(key, false, false)`. Actually, 
after this PR, it will have 2^3 = 8 possible combinations.
   
   Separating `cleanup` to two methods `closeA()` and `closeBAndC()` will be 
more clear.



-- 
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: commits-unsubscr...@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org