[GitHub] [pulsar] codelipenghui commented on a diff in pull request #20935: [improve] [ml] Persist mark deleted ops to ZK if create cursor ledger was failed

["codelipenghui (via GitHub)" <gi...@apache.org>]

codelipenghui commented on code in PR #20935:
URL: https://github.com/apache/pulsar/pull/20935#discussion_r1287029624


##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorImpl.java:
##########
@@ -2151,7 +2151,13 @@ public void operationFailed(ManagedLedgerException exception) {
 
                 mdEntry.triggerFailed(exception);
             }
-        });
+        };
+
+        if (directPersistMetadataStore) {
+            persistPositionMetaStore(mdEntry, cb);
+        } else {
+            persistPositionToLedger(cursorLedger, mdEntry, cb);
+        }

Review Comment:
   Another solution is you can change https://github.com/apache/pulsar/pull/20935/files#diff-fad355f91bd15cc041161f9a46fce62b7fee87fbfb8f0ff8a8b724a1bd1f29eeR3054
   
   to 
   
   ```
           if (lh == null) {
               persistPositionMetaStore(mdEntry, callback);
               return;
           }
   ```
   
   So that you don't need to add param `directPersistMetadataStore` to multiple methods.
   



##########
managed-ledger/src/test/java/org/apache/bookkeeper/mledger/impl/ManagedCursorTest.java:
##########
@@ -229,6 +229,28 @@ void readTwice() throws Exception {
         entries.forEach(Entry::release);
     }
 
+    @Test
+    void testPersistentMarkDeleteIfCreateCursorLedgerFailed() throws Exception {

Review Comment:
   The test covered the case that the cursor didn't have a Ledger before. Could you please help also cover the case that the cursor had a Ledger before? I think it will not work because the old Ledger will not be removed if the new Ledger creation is failed. When the broker loads the cursor again, it will try to read from the old Ledger first, which will skip all the updates that happened on the metadata store.



-- 
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-unsubscribe@pulsar.apache.org

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