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 2022/06/28 08:14:27 UTC
[GitHub] [pulsar] congbobo184 opened a new pull request, #16265: [fix][txn] Fix TopicTransactionBuffer ledger apend marker throw ManagedLedgerAlreadyClosedException
congbobo184 opened a new pull request, #16265:
URL: https://github.com/apache/pulsar/pull/16265
### Motivation
we don't handle append tb transaction marker when the ledger in `WriteFailed` throw `ManagedLedgerAlreadyClosedException.`
When ledger in `WriteFailed,` we need to close the current ledger and create a new ledger, same as https://github.com/apache/pulsar/pull/14738
### Modifications
change the ledger to the `WriteFailed` state, and it will take automatic recovery, and the commit will be successful
### Verifying this change
### Does this pull request potentially affect one of the following parts:
*If `yes` was chosen, please highlight the changes*
- Dependencies (does it add or upgrade a dependency): (no)
- The public API: (no)
- The schema: (no)
- The default values of configurations: (no)
- The wire protocol: (no)
- The rest endpoints: (no)
- The admin cli options: (no)
- Anything that affects deployment: (no)
### Documentation
- Does this pull request introduces a new feature? (yes)
- If yes, how is the feature documented? (not applicable / docs / JavaDocs / not documented)
- If a feature is not applicable for documentation, explain why?
- If a feature is not documented yet in this PR, please create a follow-up issue for adding the documentation
--
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 a diff in pull request #16265: [fix][txn] Fix TopicTransactionBuffer ledger apend marker throw ManagedLedgerAlreadyClosedException
Posted by GitBox <gi...@apache.org>.
congbobo184 commented on code in PR #16265:
URL: https://github.com/apache/pulsar/pull/16265#discussion_r910531893
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/transaction/buffer/impl/TopicTransactionBuffer.java:
##########
@@ -375,6 +377,12 @@ public void addFailed(ManagedLedgerException exception, Object ctx) {
return completableFuture;
}
+ private void checkAppendMarkerException(ManagedLedgerException exception) {
+ if (exception instanceof ManagedLedgerException.ManagedLedgerAlreadyClosedException) {
+ topic.getManagedLedger().readyToCreateNewLedger();
Review Comment:
yeah, we only need to readyToCreateNewLedger() in `State.WriteFiled`. If it in `State.Closed`, we can't update `State.WriteFiled` to `State.ClosedLedger`. Because method `readyToCreateNewLedger()` has already check the `State.WriteFiled`, so we don't need to check 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] liangyepianzhou merged pull request #16265: [fix][txn] Fix TopicTransactionBuffer ledger apend marker throw ManagedLedgerAlreadyClosedException
Posted by GitBox <gi...@apache.org>.
liangyepianzhou merged PR #16265:
URL: https://github.com/apache/pulsar/pull/16265
--
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] codelipenghui commented on a diff in pull request #16265: [fix][txn] Fix TopicTransactionBuffer ledger apend marker throw ManagedLedgerAlreadyClosedException
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on code in PR #16265:
URL: https://github.com/apache/pulsar/pull/16265#discussion_r909474975
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/transaction/buffer/impl/TopicTransactionBuffer.java:
##########
@@ -375,6 +377,12 @@ public void addFailed(ManagedLedgerException exception, Object ctx) {
return completableFuture;
}
+ private void checkAppendMarkerException(ManagedLedgerException exception) {
+ if (exception instanceof ManagedLedgerException.ManagedLedgerAlreadyClosedException) {
+ topic.getManagedLedger().readyToCreateNewLedger();
Review Comment:
Sorry, I mean if we get a `ManagedLedgerAlreadyClosedException`, it can be `State.Closed` and `State.WriteFailed`. We only need to call `readyToCreateNewLedger()` for `State.WriteFailed` but not the `State.Closed`? Because `State.Closed` means the managed ledger is closed, it not equals to the `State.ClosedLedger`
--
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] codelipenghui commented on a diff in pull request #16265: [fix][txn] Fix TopicTransactionBuffer ledger apend marker throw ManagedLedgerAlreadyClosedException
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on code in PR #16265:
URL: https://github.com/apache/pulsar/pull/16265#discussion_r908275427
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/transaction/buffer/impl/TopicTransactionBuffer.java:
##########
@@ -375,6 +377,12 @@ public void addFailed(ManagedLedgerException exception, Object ctx) {
return completableFuture;
}
+ private void checkAppendMarkerException(ManagedLedgerException exception) {
+ if (exception instanceof ManagedLedgerException.ManagedLedgerAlreadyClosedException) {
+ topic.getManagedLedger().readyToCreateNewLedger();
Review Comment:
Do we need to check the state is `State.WriteFailed`?
```
else if (state == State.Closed) {
addOperation.failed(new ManagedLedgerAlreadyClosedException("Managed ledger was already closed"));
return;
} else if (state == State.WriteFailed) {
addOperation.failed(new ManagedLedgerAlreadyClosedException("Waiting to recover from failure"));
return;
}
```
We also remove in this PR https://github.com/apache/pulsar/pull/14738/files#diff-4fb66809c520962f28616ddcab91aca0abb7aa04ee3ef0703097cf5976080ba8L166-L168
@congbobo184 Do you remember the context?
--
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 a diff in pull request #16265: [fix][txn] Fix TopicTransactionBuffer ledger apend marker throw ManagedLedgerAlreadyClosedException
Posted by GitBox <gi...@apache.org>.
congbobo184 commented on code in PR #16265:
URL: https://github.com/apache/pulsar/pull/16265#discussion_r909134599
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/transaction/buffer/impl/TopicTransactionBuffer.java:
##########
@@ -375,6 +377,12 @@ public void addFailed(ManagedLedgerException exception, Object ctx) {
return completableFuture;
}
+ private void checkAppendMarkerException(ManagedLedgerException exception) {
+ if (exception instanceof ManagedLedgerException.ManagedLedgerAlreadyClosedException) {
+ topic.getManagedLedger().readyToCreateNewLedger();
Review Comment:
`topic.getManagedLedger().readyToCreateNewLedger();` this method is
```
if (STATE_UPDATER.compareAndSet(this, State.WriteFailed, State.ClosedLedger)){
log.info("[{}] Managed ledger is now ready to accept writes again", name);
}
```
so we don't need to check the state is `State.WriteFailed`
--
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