You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2021/09/21 07:08:32 UTC
[GitHub] [pulsar] lordcheng10 opened a new pull request #12113: Remove unused method: asyncCloseCursorLedger
lordcheng10 opened a new pull request #12113:
URL: https://github.com/apache/pulsar/pull/12113
The org.apache.bookkeeper.mledger.impl.ManagedCursorImpl#asyncCloseCursorLedger method is an unused method, delete the redundant method
--
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
[GitHub] [pulsar] lordcheng10 commented on pull request #12113: Use asyncCloseCursorLedger to replace cursorLedger.asyncClose method in the ManagedCursorImpl.VoidCallback#operationComplete
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on pull request #12113:
URL: https://github.com/apache/pulsar/pull/12113#issuecomment-929748893
CI has passed @BewareMyPower @michaeljmarshall PTAL ，thanks！
--
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
[GitHub] [pulsar] lordcheng10 edited a comment on pull request #12113: Remove unused method: asyncCloseCursorLedger
Posted by GitBox <gi...@apache.org>.
lordcheng10 edited a comment on pull request #12113:
URL: https://github.com/apache/pulsar/pull/12113#issuecomment-927233753
@codelipenghui @lhotari
Hi, do you have any suggestions for this discussion
--
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
[GitHub] [pulsar] lordcheng10 commented on pull request #12113: Remove unused method: asyncCloseCursorLedger
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on pull request #12113:
URL: https://github.com/apache/pulsar/pull/12113#issuecomment-927233753
@codelipenghui @lhotari do you have any suggestions for this discussion
--
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
[GitHub] [pulsar] lordcheng10 edited a comment on pull request #12113: Remove unused method: asyncCloseCursorLedger
Posted by GitBox <gi...@apache.org>.
lordcheng10 edited a comment 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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Anonymitaet commented on pull request #12113: Remove unused method: asyncCloseCursorLedger
Posted by GitBox <gi...@apache.org>.
Anonymitaet commented on pull request #12113:
URL: https://github.com/apache/pulsar/pull/12113#issuecomment-924553807
@lordcheng10 Thanks for your contribution. For this PR, do we need to update docs?
(The [PR template contains info about doc](https://github.com/apache/pulsar/blob/master/.github/PULL_REQUEST_TEMPLATE.md#documentation), which helps others know more about the changes. Can you provide doc-related info in this and future PR descriptions? Thanks)
--
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
[GitHub] [pulsar] lordcheng10 edited a comment on pull request #12113: Remove unused method: asyncCloseCursorLedger
Posted by GitBox <gi...@apache.org>.
lordcheng10 edited a comment on pull request #12113:
URL: https://github.com/apache/pulsar/pull/12113#issuecomment-925512359
--
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
[GitHub] [pulsar] congbobo184 merged pull request #12113: Use asyncCloseCursorLedger to replace cursorLedger.asyncClose method in the ManagedCursorImpl.VoidCallback#operationComplete
Posted by GitBox <gi...@apache.org>.
congbobo184 merged pull request #12113:
URL: https://github.com/apache/pulsar/pull/12113
--
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
[GitHub] [pulsar] lordcheng10 commented on pull request #12113: Use asyncCloseCursorLedger to replace cursorLedger.asyncClose method in the ManagedCursorImpl.VoidCallback#operationComplete
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on pull request #12113:
URL: https://github.com/apache/pulsar/pull/12113#issuecomment-943165034
> 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
@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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lordcheng10 commented on pull request #12113: Use asyncCloseCursorLedger to replace cursorLedger.asyncClose method in the ManagedCursorImpl.VoidCallback#operationComplete
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on pull request #12113:
URL: https://github.com/apache/pulsar/pull/12113#issuecomment-943152278
> 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
--
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
[GitHub] [pulsar] lordcheng10 edited a comment on pull request #12113: Use asyncCloseCursorLedger to replace cursorLedger.asyncClose method in the ManagedCursorImpl.VoidCallback#operationComplete
Posted by GitBox <gi...@apache.org>.
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
--
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
[GitHub] [pulsar] lordcheng10 edited a comment on pull request #12113: Remove unused method: asyncCloseCursorLedger
Posted by GitBox <gi...@apache.org>.
lordcheng10 edited a comment on pull request #12113:
URL: https://github.com/apache/pulsar/pull/12113#issuecomment-927233753
@codelipenghui @lhotari @merlimat @BewareMyPower - PTAL, thanks.
--
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
[GitHub] [pulsar] lordcheng10 removed a comment on pull request #12113: Remove unused method: asyncCloseCursorLedger
Posted by GitBox <gi...@apache.org>.
lordcheng10 removed a comment on pull request #12113:
URL: https://github.com/apache/pulsar/pull/12113#issuecomment-927233753
@codelipenghui @lhotari @merlimat @BewareMyPower - PTAL, thanks.
--
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
[GitHub] [pulsar] BewareMyPower commented on pull request #12113: Use asyncCloseCursorLedger to replace cursorLedger.asyncClose method in the ManagedCursorImpl.VoidCallback#operationComplete
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on pull request #12113:
URL: https://github.com/apache/pulsar/pull/12113#issuecomment-929753811
@congbobo184 could you answer the question from @michaeljmarshall ?
--
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
[GitHub] [pulsar] lordcheng10 commented on pull request #12113: Use asyncCloseCursorLedger to replace cursorLedger.asyncClose method in the ManagedCursorImpl.VoidCallback#operationComplete
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented 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 said, I added two logs in asyncCloseCursorLedger
--
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
[GitHub] [pulsar] michaeljmarshall commented on pull request #12113: Remove unused method: asyncCloseCursorLedger
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on pull request #12113:
URL: https://github.com/apache/pulsar/pull/12113#issuecomment-926055744
I agree that it seems appropriate to use `asyncCloseCursorLedger` to replace the code in the `operationComplete` method. My only question is if failing the callback is the right behavior. It seems correct to me, but I'd like to verify with someone more familiar with this part of the code base.
--
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
[GitHub] [pulsar] BewareMyPower commented on pull request #12113: Use asyncCloseCursorLedger to replace cursorLedger.asyncClose method in the ManagedCursorImpl.VoidCallback#operationComplete
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on pull request #12113:
URL: https://github.com/apache/pulsar/pull/12113#issuecomment-929753811
@congbobo184 could you answer the question from @michaeljmarshall ?
--
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
[GitHub] [pulsar] lordcheng10 edited a comment on pull request #12113: Remove unused method: asyncCloseCursorLedger
Posted by GitBox <gi...@apache.org>.
lordcheng10 edited a comment 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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lordcheng10 commented on pull request #12113: Use asyncCloseCursorLedger to replace cursorLedger.asyncClose method in the ManagedCursorImpl.VoidCallback#operationComplete
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on pull request #12113:
URL: https://github.com/apache/pulsar/pull/12113#issuecomment-928606870
--
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
[GitHub] [pulsar] lordcheng10 edited a comment on pull request #12113: Use asyncCloseCursorLedger to replace cursorLedger.asyncClose method in the ManagedCursorImpl.VoidCallback#operationComplete
Posted by GitBox <gi...@apache.org>.
lordcheng10 edited a comment on pull request #12113:
URL: https://github.com/apache/pulsar/pull/12113#issuecomment-943165034
> 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
@congbobo184 fix bug: 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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] congbobo184 edited a comment on pull request #12113: Use asyncCloseCursorLedger to replace cursorLedger.asyncClose method in the ManagedCursorImpl.VoidCallback#operationComplete
Posted by GitBox <gi...@apache.org>.
congbobo184 edited a comment on pull request #12113:
URL: https://github.com/apache/pulsar/pull/12113#issuecomment-943139037
LGTM！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
--
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
[GitHub] [pulsar] lordcheng10 commented on pull request #12113: Use asyncCloseCursorLedger to replace cursorLedger.asyncClose method in the ManagedCursorImpl.VoidCallback#operationComplete
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on pull request #12113:
URL: https://github.com/apache/pulsar/pull/12113#issuecomment-929748893
CI has passed @BewareMyPower @michaeljmarshall PTAL ，thanks！
--
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
[GitHub] [pulsar] lordcheng10 commented on pull request #12113: Remove unused method: asyncCloseCursorLedger
Posted by GitBox <gi...@apache.org>.
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lordcheng10 commented on pull request #12113: Remove unused method: asyncCloseCursorLedger
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on pull request #12113:
URL: https://github.com/apache/pulsar/pull/12113#issuecomment-924889972
> @lordcheng10 Thanks for your contribution. For this PR, do we need to update docs?
>
> (The [PR template contains info about doc](https://github.com/apache/pulsar/blob/master/.github/PULL_REQUEST_TEMPLATE.md#documentation), which helps others know more about the changes. Can you provide doc-related info in this and future PR descriptions? Thanks)
@Anonymitaet I searched for the document about this method in the project, but did not find any document related to this method, so there is no need to modify the document.
--
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
[GitHub] [pulsar] lordcheng10 removed a comment on pull request #12113: Remove unused method: asyncCloseCursorLedger
Posted by GitBox <gi...@apache.org>.
lordcheng10 removed a comment on pull request #12113:
URL: https://github.com/apache/pulsar/pull/12113#issuecomment-926383003
> Updated md-position={} into cu
@merlimat Do you have any good suggestions for this discussion？
--
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
[GitHub] [pulsar] lordcheng10 edited a comment on pull request #12113: Use asyncCloseCursorLedger to replace cursorLedger.asyncClose method in the ManagedCursorImpl.VoidCallback#operationComplete
Posted by GitBox <gi...@apache.org>.
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
--
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
[GitHub] [pulsar] lordcheng10 commented on pull request #12113: Remove unused method: asyncCloseCursorLedger
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on pull request #12113:
URL: https://github.com/apache/pulsar/pull/12113#issuecomment-924619129
> @lordcheng10 Thanks for your contribution. For this PR, do we need to update docs?
>
> (The [PR template contains info about doc](https://github.com/apache/pulsar/blob/master/.github/PULL_REQUEST_TEMPLATE.md#documentation), which helps others know more about the changes. Can you provide doc-related info in this and future PR descriptions? Thanks)
OK, I'd be happy to update the document
--
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
[GitHub] [pulsar] lordcheng10 edited a comment on pull request #12113: Remove unused method: asyncCloseCursorLedger
Posted by GitBox <gi...@apache.org>.
lordcheng10 edited a comment on pull request #12113:
URL: https://github.com/apache/pulsar/pull/12113#issuecomment-927233753
@codelipenghui @lhotari @merlimat - PTAL, thanks.
--
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
[GitHub] [pulsar] lordcheng10 commented on pull request #12113: Remove unused method: asyncCloseCursorLedger
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on pull request #12113:
URL: https://github.com/apache/pulsar/pull/12113#issuecomment-926383003
> Updated md-position={} into cu
@merlimat Do you have any good suggestions for this discussion？
--
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
[GitHub] [pulsar] lordcheng10 edited a comment on pull request #12113: Remove unused method: asyncCloseCursorLedger
Posted by GitBox <gi...@apache.org>.
lordcheng10 edited a comment 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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lordcheng10 commented on pull request #12113: Remove unused method: asyncCloseCursorLedger
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on pull request #12113:
URL: https://github.com/apache/pulsar/pull/12113#issuecomment-926383573
> > 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);
> }
> ```
@merlimat Do you have any good suggestions for this discussion?
--
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
[GitHub] [pulsar] lordcheng10 removed a comment on pull request #12113: Remove unused method: asyncCloseCursorLedger
Posted by GitBox <gi...@apache.org>.
lordcheng10 removed a comment on pull request #12113:
URL: https://github.com/apache/pulsar/pull/12113#issuecomment-926383573
> > 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);
> }
> ```
@merlimat Do you have any good suggestions for this discussion?
--
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
[GitHub] [pulsar] congbobo184 commented on pull request #12113: Use asyncCloseCursorLedger to replace cursorLedger.asyncClose method in the ManagedCursorImpl.VoidCallback#operationComplete
Posted by GitBox <gi...@apache.org>.
congbobo184 commented on pull request #12113:
URL: https://github.com/apache/pulsar/pull/12113#issuecomment-943139037
LGTM！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.
--
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
[GitHub] [pulsar] lordcheng10 edited a comment on pull request #12113: Remove unused method: asyncCloseCursorLedger
Posted by GitBox <gi...@apache.org>.
lordcheng10 edited a comment 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() {
\tlog.info("[{}][{}] Updated md-position={} into cursor-ledger {}", ledger.getName(), name,markDeletePosition,
\tcursorLedger.getId());
\tasyncCloseCursorLedger(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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lordcheng10 commented on pull request #12113: Use asyncCloseCursorLedger to replace cursorLedger.asyncClose method in the ManagedCursorImpl.VoidCallback#operationComplete
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on pull request #12113:
URL: https://github.com/apache/pulsar/pull/12113#issuecomment-928916720
@eolivelli, @codelipenghui, @BewareMyPower, @sijie, @hangc0276, @merlimat - PTAL, thanks.
--
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
[GitHub] [pulsar] lordcheng10 commented on pull request #12113: Use asyncCloseCursorLedger to replace cursorLedger.asyncClose method in the ManagedCursorImpl.VoidCallback#operationComplete
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on pull request #12113:
URL: https://github.com/apache/pulsar/pull/12113#issuecomment-940929104
@congbobo184 PTAL ,thanks!
--
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
[GitHub] [pulsar] lordcheng10 edited a comment on pull request #12113: Use asyncCloseCursorLedger to replace cursorLedger.asyncClose method in the ManagedCursorImpl.VoidCallback#operationComplete
Posted by GitBox <gi...@apache.org>.
lordcheng10 edited a comment on pull request #12113:
URL: https://github.com/apache/pulsar/pull/12113#issuecomment-928606870
--
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
[GitHub] [pulsar] lordcheng10 commented on pull request #12113: Remove unused method: asyncCloseCursorLedger
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on pull request #12113:
URL: https://github.com/apache/pulsar/pull/12113#issuecomment-927485031
> > 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);
> }
> ```
@codelipenghui @lhotari @merlimat @BewareMyPower - PTAL, thanks.
--
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
[GitHub] [pulsar] lordcheng10 commented on pull request #12113: Use asyncCloseCursorLedger to replace cursorLedger.asyncClose method in the ManagedCursorImpl.VoidCallback#operationComplete
Posted by GitBox <gi...@apache.org>.
lordcheng10 commented on pull request #12113:
URL: https://github.com/apache/pulsar/pull/12113#issuecomment-929401486
/pulsarbot run-failure-checks
--
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
[GitHub] [pulsar] lordcheng10 edited a comment on pull request #12113: Use asyncCloseCursorLedger to replace cursorLedger.asyncClose method in the ManagedCursorImpl.VoidCallback#operationComplete
Posted by GitBox <gi...@apache.org>.
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
--
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
[GitHub] [pulsar] lordcheng10 edited a comment on pull request #12113: Use asyncCloseCursorLedger to replace cursorLedger.asyncClose method in the ManagedCursorImpl.VoidCallback#operationComplete
Posted by GitBox <gi...@apache.org>.
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org