lordcheng10 edited a comment on pull request #12113: URL: https://github.com/apache/pulsar/pull/12113#issuecomment-928606870
> LGTM. From my perspective, this PR changes the callback behavior to the _expected_ behavior. We should call the `closeFailed` callback method when we fail to close the cursor ledger. > > @lordcheng10 - now that we're changing direction on this PR, we should update the title so that the commit message will align with the actual change. It might also be a good idea to update the `asyncCloseCursorLedger` method to log the two removed log lines. > > Based on quickly looking at usage for the `persistPositionWhenClosing` method, it looks like the only code that will change fundamental behavior is here: > > https://github.com/apache/pulsar/blob/04604724dd89a4c8ff40b922e80c7045fedf112d/pulsar-broker/src/main/java/org/apache/pulsar/broker/transaction/pendingack/impl/MLPendingAckStore.java#L120-L135 > > No other methods have branching behavior in their callback based on `closeComplete` vs `closeFailure`. > > @congbobo184 - you wrote the above code block for transactions. Will it be a problem to change behavior as this PR proposes? As you described, I added two logs in asyncCloseCursorLedger and updated the title > > LGTM！[#12113 (comment)](https://github.com/apache/pulsar/pull/12113#issuecomment-928606870) this seem to a bug. This is a bug, can you fix it easily? Future completed twice.thanks > > Yeah. I will fix @congbobo184 https://github.com/apache/pulsar/pull/12362 -- 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]
