lordcheng10 commented on pull request #12113: URL: https://github.com/apache/pulsar/pull/12113#issuecomment-925512359
> It almost seems to me that this method should have been used to close the cursor ledger here: > > https://github.com/apache/pulsar/blob/0a1ae983ebb9fb049bbf34bfa3fe55f33101c9d6/managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorImpl.java#L2235-L2250 > > . > The logic of this `asyncCloseCursorLedger` method is quite similar, but not identical, to the above code block. Specifically, this `asyncCloseCursorLedger` method has metrics and fails the callback if the close fails. I wonder if its worth updating the `persistPositionWhenClosing` method to use this currently unused method. > > If we do decide to remove `asyncCloseCursorLedger`, I think if we should either update `persistPositionWhenClosing` to increment/decrement the `cursorLedgerCloseOp` or remove the `cursorLedgerCloseOp` metric, as well as its references, from the `ledger.mbean`, since it is only updated within this method. > > @eolivelli - do you have any thoughts on the right direction here? Thanks. @michaeljmarshall It seems that it is better to use asyncCloseCursorLedger to replace the asynchronous close in operationComplete. The modified operationComplete should look like this: public void operationComplete() { log.info("[{}][{}] Updated md-position={} into cursor-ledger {}", ledger.getName(), name,markDeletePosition, cursorLedger.getId()); asyncCloseCursorLedger(callback, ctx); } -- 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]
