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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
