[GitHub] [pulsar] lhotari commented on a change in pull request #10480: [Broker] Fix race condition in invalidating ledger cache entries

[GitBox <gi...@apache.org>]

lhotari commented on a change in pull request #10480:
URL: https://github.com/apache/pulsar/pull/10480#discussion_r626277254



##########
File path: managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/EntryImpl.java
##########
@@ -166,7 +170,16 @@ protected void deallocate() {
         timestamp = -1;
         ledgerId = -1;
         entryId = -1;
+        invalidated.set(false);
         recyclerHandle.recycle(this);
     }
 
+    @Override
+    public boolean invalidate() {
+        if (invalidated.compareAndSet(false, true)) {
+            release();
+            return true;
+        }
+        return false;

Review comment:
       This is a safety measure to prevent races in invalidating the entries. I agree that the issues in releasing must be fixed. The benefit of adding a separate method for invalidation would help detect when the problem is caused by invalidating the entry twice. Some logging could be added to detect the issues where there's a race in invalidation which causes a "double release".
   
   Currently, it seems that the problems that we are seeing could occur only when there's a race in invalidation. At a quick glance, there doesn't seem to be other code paths where the entry is released but not retained as part of the same "flow". 
   
   Would this justify adding some extra protection against races in invalidation?




-- 
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.

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