Re: [PR] [fix][ml] Prevent durable cursor reset from being silently discarded by a concurrent individual delete [pulsar]

["dao-jun (via GitHub)" <gi...@apache.org>]

dao-jun commented on PR #26318:
URL: https://github.com/apache/pulsar/pull/26318#issuecomment-5743983001

   Blocker (non-durable cursor regression): internalResetCursor now calls the new 6-arg internalAsyncMarkDelete(..., isCursorReset=true) directly. NonDurableCursorImpl only overrides the 5-arg overload, so a reset on a non-durable cursor no longer dispatches to its sync, persistence-free implementation — it falls into the durable path with cursorLedger == null and State.Open, ending in an NPE inside persistPositionToLedger (or in the reset being queued behind reads). The seek callback never fires and RESET_CURSOR_IN_PROGRESS stays armed forever. NonDurableCursorTest#testResetCursor / #testasyncResetCursor cover exactly this and should fail on the current head. Suggestion: override the 6-arg overload in NonDurableCursorImpl (or keep internalResetCursor on the 5-arg call and pass the reset marker another way). Also note ManagedLedgerTest stubs the 5-arg overload — any stub covering the reset path will silently stop matching.
   
   Non-blocker (rejected acks still mutate state): The early flag checks at the top of asyncDelete/asyncMarkDelete were removed, so an ack racing a reset now applies its synchronous mutations (individualDeletedMessages, setAcknowledgedPosition) before being rejected under the monitor. On a successful reset the runnable heals memory, but the flush persist reads the live individualDeletedMessages, so the rejected ack's positions are baked into the durable snapshot — after a restart, messages whose ack failed are durably individually-deleted (an at-most-once window for e.g. transaction pending-ack commit). Consider restoring a best-effort flag read at the top of both entry points (reject before mutating) in addition to the atomic check under pendingMarkDeleteOps.
   
   Also: the PR description still describes the v1 approach (top-of-asyncDelete guard) rather than the monitor serialization actually on the head — it needs a rewrite before merge. 


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