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/10/24 14:27:39 UTC
[GitHub] [pulsar] poorbarcode opened a new pull request, #18123: [test][ml]Add test:Ensure that cursor data is not lost if the recover timeout fails and the retry succeeds
poorbarcode opened a new pull request, #18123:
URL: https://github.com/apache/pulsar/pull/18123
### Motivation
When the broker restarts, the cursor will run `recover` to recover ack data. If the state `cursorLedger` is `IN_RECOVERY`, `curosor.recover` will fail and data will be recovered from ZK. This causes cursor data to be lost, causing the consumer to consume a large number of duplicate messages.
The execution process of the problem is as follows:
- cursor.recover()
- open `cursorLedger`
- read ledger meta success
- try reading entries from two BK, but all timeout ( code =-23 )
- recover from ledger failure, then recover from ZK
Log:
```
Oct 17, 2022 22:54:05.818 [BookKeeperClientWorker-OrderedExecutor-16-0] ERROR org.apache.bookkeeper.client.ReadLastConfirmedOp - While readLastConfirmed ledger: 59158316 did not hear success responses from all quorums, QuorumCoverage(e:2,w:2,a:2) = [-23, -23]
Oct 17, 2022 22:54:05.818 [BookKeeperClientWorker-OrderedExecutor-16-0] ERROR org.apache.bookkeeper.mledger.impl.ManagedCursorImpl - [order/org-217/persistent/p1010-tx] Error opening metadata ledger 59158316 for consumer order-service: Error while recovering ledger
Oct 17, 2022 22:54:05.818 [BookKeeperClientWorker-OrderedExecutor-16-0] INFO org.apache.bookkeeper.mledger.impl.ManagedCursorImpl - [order/org-217/persistent/p1010-tx] Opened ledger 59158316 for consumer order-service. rc=-10
```
Looking at the ledger metadata:
```
LedgerMetadata{formatVersion=3, ensembleSize=2, writeQuorumSize=2, ackQuorumSize=2, state=IN_RECOVERY, digestType=CRC32C, password=base64:, ensembles={0=[***:3181, ***:3181]}, customMetadata={component=***, pulsar/managed-ledger=***, pulsar/cursor=***, application=***}}
```
<!-- Explain here the context, and why you're making that change. What is the problem you're trying to solve. -->
### Modifications
- Add test: `cursor.recover` will fail by `timeoutEx`, and cursor data will not be lost if the retry succeeds. <strong>(High light)</strong> The BK-PR below will make the timeout ex use a separate error code, leading the timeout ex to be recoverable.
- https://github.com/apache/bookkeeper/pull/3562
- Add multiple tests for the `ManagedCursorImpl.recover()`
### Documentation
<!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. -->
- [ ] `doc` <!-- Your PR contains doc changes. Please attach the local preview screenshots (run `sh start.sh` at `pulsar/site2/website`) to your PR description, or else your PR might not get merged. -->
- [ ] `doc-required` <!-- Your PR changes impact docs and you will update later -->
- [x] `doc-not-needed` <!-- Your PR changes do not impact docs -->
- [ ] `doc-complete` <!-- Docs have been already added -->
### Matching PR in forked repository
PR in forked repository:
- https://github.com/poorbarcode/pulsar/pull/28
--
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] mattisonchao commented on a diff in pull request #18123: [improve][ml]Make cursor-recover retry when ledger-recover timeout
Posted by GitBox <gi...@apache.org>.
mattisonchao commented on code in PR #18123:
URL: https://github.com/apache/pulsar/pull/18123#discussion_r1000101327
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorImpl.java:
##########
@@ -500,6 +502,15 @@ protected void recoverFromLedger(final ManagedCursorInfo info, final VoidCallbac
if (log.isInfoEnabled()) {
log.info("[{}] Opened ledger {} for consumer {}. rc={}", ledger.getName(), ledgerId, name, rc);
}
+ // If ledgerMeta exists, it means ledger is not deleted normally and bookie is trying to recover the data.
+ // We will make several attempts to ensure that do not lose cursor data.
+ if (maybeLedgerRecovering(rc) && retryTimesWhenLedgerRecovering > 0) {
+ log.warn("[{}] Opened ledger {} for consumer {}. rc={}. Ledger maybe is recovering,"
+ + " try to recover again, at most try {} times", ledger.getName(), ledgerId, name, rc,
+ retryTimesWhenLedgerRecovering - 1);
+ recoverFromLedger(info, callback, retryTimesWhenLedgerRecovering - 1);
+ return;
+ }
Review Comment:
Yes, agree with this suggestion. Because retrying twice may not solve the problem. It just reduces the problem.
> if users don't want to skip the non-recoverable data.
For this case, I would like to add a new config to avoid changing the original behaviour. (maybe need to discuss this here?)
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorImpl.java:
##########
@@ -480,7 +481,7 @@ public void operationComplete(ManagedCursorInfo info, Stat stat) {
// Need to proceed and read the last entry in the specified ledger to find out the last position
log.info("[{}] Consumer {} meta-data recover from ledger {}", ledger.getName(), name,
info.getCursorsLedgerId());
- recoverFromLedger(info, callback);
+ recoverFromLedger(info, callback, 2);
Review Comment:
why hardcode 2 here?
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorImpl.java:
##########
@@ -500,6 +502,15 @@ protected void recoverFromLedger(final ManagedCursorInfo info, final VoidCallbac
if (log.isInfoEnabled()) {
log.info("[{}] Opened ledger {} for consumer {}. rc={}", ledger.getName(), ledgerId, name, rc);
}
+ // If ledgerMeta exists, it means ledger is not deleted normally and bookie is trying to recover the data.
+ // We will make several attempts to ensure that do not lose cursor data.
+ if (maybeLedgerRecovering(rc) && retryTimesWhenLedgerRecovering > 0) {
+ log.warn("[{}] Opened ledger {} for consumer {}. rc={}. Ledger maybe is recovering,"
+ + " try to recover again, at most try {} times", ledger.getName(), ledgerId, name, rc,
+ retryTimesWhenLedgerRecovering - 1);
+ recoverFromLedger(info, callback, retryTimesWhenLedgerRecovering - 1);
+ return;
+ }
Review Comment:
Yes, agree with this suggestion. Because retrying twice may not solve the problem. It just reduces the problem.
> if users don't want to skip the non-recoverable data.
For this case, I would like to add a new config to avoid changing the original behaviour. (maybe need to discuss this here?)
--
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] poorbarcode commented on a diff in pull request #18123: [test][ml]Add test:Ensure that cursor data is not lost if the recover timeout fails and the retry succeeds
Posted by GitBox <gi...@apache.org>.
poorbarcode commented on code in PR #18123:
URL: https://github.com/apache/pulsar/pull/18123#discussion_r1000713957
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorImpl.java:
##########
@@ -500,6 +502,15 @@ protected void recoverFromLedger(final ManagedCursorInfo info, final VoidCallbac
if (log.isInfoEnabled()) {
log.info("[{}] Opened ledger {} for consumer {}. rc={}", ledger.getName(), ledgerId, name, rc);
}
+ // If ledgerMeta exists, it means ledger is not deleted normally and bookie is trying to recover the data.
+ // We will make several attempts to ensure that do not lose cursor data.
+ if (maybeLedgerRecovering(rc) && retryTimesWhenLedgerRecovering > 0) {
+ log.warn("[{}] Opened ledger {} for consumer {}. rc={}. Ledger maybe is recovering,"
+ + " try to recover again, at most try {} times", ledger.getName(), ledgerId, name, rc,
+ retryTimesWhenLedgerRecovering - 1);
+ recoverFromLedger(info, callback, retryTimesWhenLedgerRecovering - 1);
+ return;
+ }
Review Comment:
Hi @codelipenghui @mattisonchao
> I think treating the TimeoutException as a recoverable error is simple here. We are not able to load the data from the bookkeeper now. Just let the topic load get failed. The client side will trigger the topic loading again.
Good idea. I have submit a PR to make the timeout ex use a separate error code
- https://github.com/apache/bookkeeper/pull/3562
--
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] codecov-commenter commented on pull request #18123: [test][ml]Add test to ensure that cursor data is not lost if the recover timeout fails
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #18123:
URL: https://github.com/apache/pulsar/pull/18123#issuecomment-1289250184
# [Codecov](https://codecov.io/gh/apache/pulsar/pull/18123?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#18123](https://codecov.io/gh/apache/pulsar/pull/18123?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (da85ea9) into [master](https://codecov.io/gh/apache/pulsar/commit/6c65ca0d8a80bfaaa4d5869e0cea485f5c94369b?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (6c65ca0) will **decrease** coverage by `9.37%`.
> The diff coverage is `49.09%`.
[](https://codecov.io/gh/apache/pulsar/pull/18123?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #18123 +/- ##
============================================
- Coverage 34.91% 25.54% -9.38%
+ Complexity 5707 3245 -2462
============================================
Files 607 372 -235
Lines 53396 41306 -12090
Branches 5712 4229 -1483
============================================
- Hits 18644 10551 -8093
+ Misses 32119 29028 -3091
+ Partials 2633 1727 -906
```
| Flag | Coverage Δ | |
|---|---|---|
| unittests | `25.54% <49.09%> (-9.38%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/pulsar/pull/18123?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [.../pulsar/broker/admin/impl/SchemasResourceBase.java](https://codecov.io/gh/apache/pulsar/pull/18123/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9hZG1pbi9pbXBsL1NjaGVtYXNSZXNvdXJjZUJhc2UuamF2YQ==) | `38.38% <0.00%> (-7.08%)` | :arrow_down: |
| [.../org/apache/pulsar/broker/admin/v2/Namespaces.java](https://codecov.io/gh/apache/pulsar/pull/18123/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9hZG1pbi92Mi9OYW1lc3BhY2VzLmphdmE=) | `10.65% <0.00%> (+2.63%)` | :arrow_up: |
| [.../pulsar/broker/service/AbstractBaseDispatcher.java](https://codecov.io/gh/apache/pulsar/pull/18123/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9zZXJ2aWNlL0Fic3RyYWN0QmFzZURpc3BhdGNoZXIuamF2YQ==) | `34.35% <ø> (-11.51%)` | :arrow_down: |
| [...che/pulsar/broker/service/BacklogQuotaManager.java](https://codecov.io/gh/apache/pulsar/pull/18123/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9zZXJ2aWNlL0JhY2tsb2dRdW90YU1hbmFnZXIuamF2YQ==) | `9.09% <0.00%> (-0.40%)` | :arrow_down: |
| [.../service/SystemTopicBasedTopicPoliciesService.java](https://codecov.io/gh/apache/pulsar/pull/18123/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9zZXJ2aWNlL1N5c3RlbVRvcGljQmFzZWRUb3BpY1BvbGljaWVzU2VydmljZS5qYXZh) | `53.98% <0.00%> (+2.39%)` | :arrow_up: |
| [.../pulsar/broker/stats/BrokerOperabilityMetrics.java](https://codecov.io/gh/apache/pulsar/pull/18123/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9zdGF0cy9Ccm9rZXJPcGVyYWJpbGl0eU1ldHJpY3MuamF2YQ==) | `98.21% <ø> (+5.56%)` | :arrow_up: |
| [...g/apache/pulsar/compaction/CompactedTopicImpl.java](https://codecov.io/gh/apache/pulsar/pull/18123/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2NvbXBhY3Rpb24vQ29tcGFjdGVkVG9waWNJbXBsLmphdmE=) | `11.42% <0.00%> (+0.71%)` | :arrow_up: |
| [...sistent/PersistentDispatcherMultipleConsumers.java](https://codecov.io/gh/apache/pulsar/pull/18123/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9zZXJ2aWNlL3BlcnNpc3RlbnQvUGVyc2lzdGVudERpc3BhdGNoZXJNdWx0aXBsZUNvbnN1bWVycy5qYXZh) | `40.14% <28.57%> (-11.56%)` | :arrow_down: |
| [...n/java/org/apache/pulsar/broker/PulsarService.java](https://codecov.io/gh/apache/pulsar/pull/18123/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9QdWxzYXJTZXJ2aWNlLmphdmE=) | `54.66% <50.00%> (+0.33%)` | :arrow_up: |
| [...broker/delayed/InMemoryDelayedDeliveryTracker.java](https://codecov.io/gh/apache/pulsar/pull/18123/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9kZWxheWVkL0luTWVtb3J5RGVsYXllZERlbGl2ZXJ5VHJhY2tlci5qYXZh) | `22.00% <50.00%> (+22.00%)` | :arrow_up: |
| ... and [356 more](https://codecov.io/gh/apache/pulsar/pull/18123/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
--
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] poorbarcode commented on pull request #18123: [improve][ml]Make cursor-recover retry when ledger-recover timeout
Posted by GitBox <gi...@apache.org>.
poorbarcode commented on PR #18123:
URL: https://github.com/apache/pulsar/pull/18123#issuecomment-1284500957
this patch should merge into these branches:
- master
- 2.11
- 2.10
- 2.9
- 2.8
--
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] poorbarcode commented on a diff in pull request #18123: [test][ml]Add test:Ensure that cursor data is not lost if the recover timeout fails and the retry succeeds
Posted by GitBox <gi...@apache.org>.
poorbarcode commented on code in PR #18123:
URL: https://github.com/apache/pulsar/pull/18123#discussion_r1000716069
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorImpl.java:
##########
@@ -480,7 +481,7 @@ public void operationComplete(ManagedCursorInfo info, Stat stat) {
// Need to proceed and read the last entry in the specified ledger to find out the last position
log.info("[{}] Consumer {} meta-data recover from ledger {}", ledger.getName(), name,
info.getCursorsLedgerId());
- recoverFromLedger(info, callback);
+ recoverFromLedger(info, callback, 2);
Review Comment:
Thanks for the comment. The modification has changed. see https://github.com/apache/pulsar/pull/18123#discussion_r1000713957. Could you take a look again?
--
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 #18123: [improve][ml]Make cursor-recover retry when ledger-recover timeout
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on code in PR #18123:
URL: https://github.com/apache/pulsar/pull/18123#discussion_r1000082416
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorImpl.java:
##########
@@ -500,6 +502,15 @@ protected void recoverFromLedger(final ManagedCursorInfo info, final VoidCallbac
if (log.isInfoEnabled()) {
log.info("[{}] Opened ledger {} for consumer {}. rc={}", ledger.getName(), ledgerId, name, rc);
}
+ // If ledgerMeta exists, it means ledger is not deleted normally and bookie is trying to recover the data.
+ // We will make several attempts to ensure that do not lose cursor data.
+ if (maybeLedgerRecovering(rc) && retryTimesWhenLedgerRecovering > 0) {
+ log.warn("[{}] Opened ledger {} for consumer {}. rc={}. Ledger maybe is recovering,"
+ + " try to recover again, at most try {} times", ledger.getName(), ledgerId, name, rc,
+ retryTimesWhenLedgerRecovering - 1);
+ recoverFromLedger(info, callback, retryTimesWhenLedgerRecovering - 1);
+ return;
+ }
Review Comment:
Looks like we can just add the `TimeoutException` to the recoverable error list.
```java
public static boolean isBkErrorNotRecoverable(int rc) {
switch (rc) {
case Code.NoSuchLedgerExistsException:
case Code.NoSuchLedgerExistsOnMetadataServerException:
case Code.ReadException:
case Code.LedgerRecoveryException:
case Code.NoSuchEntryException:
return true;
default:
return false;
}
}
```
We introduced a new retry mechanism, but if we reach the max retry count, the subscription will rewind to the oldest available entry.
This is unexpected from the user's perspective, we should not skip the recovery from the bookkeeper if users don't want to skip the non-recoverable data.
I think treating the `TimeoutException` as a recoverable error is simple here. We are not able to load the data from the bookkeeper now. Just let the topic load get failed. The client side will trigger the topic loading again.
--
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] Technoboy- closed pull request #18123: [test][ml]Add test:Ensure that cursor data is not lost if the recover timeout fails and the retry succeeds
Posted by GitBox <gi...@apache.org>.
Technoboy- closed pull request #18123: [test][ml]Add test:Ensure that cursor data is not lost if the recover timeout fails and the retry succeeds
URL: https://github.com/apache/pulsar/pull/18123
--
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
Re: [PR] [improve] [test] Add test to ensure that cursor data is not lost if the recover timeout fails [pulsar]
Posted by "merlimat (via GitHub)" <gi...@apache.org>.
merlimat commented on PR #18123:
URL: https://github.com/apache/pulsar/pull/18123#issuecomment-6357526776
Thanks @poorbarcode. Since this was opened, master changed: timeouts on cursor-ledger open/read now fail recovery instead of rolling back (`isBkErrorNotRecoverable`), and the rollback paths are covered in `ManagedCursorTest`. This test no longer compiles (`PositionImpl` was removed), and its `asyncOpenLedger` mock isn't reached now that we use `newOpenLedgerOp()`. I'm closing it. If you'd like to keep the "transient error, then retry, no lost acks" case, a small test in `ManagedCursorTest` using `bkc.failAfter(..., TimeoutException)` would be welcome as a new PR.
--
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
Re: [PR] [improve] [test] Add test to ensure that cursor data is not lost if the recover timeout fails [pulsar]
Posted by "merlimat (via GitHub)" <gi...@apache.org>.
merlimat closed pull request #18123: [improve] [test] Add test to ensure that cursor data is not lost if the recover timeout fails
URL: https://github.com/apache/pulsar/pull/18123
--
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] github-actions[bot] commented on pull request #18123: [improve] [test] Add test to ensure that cursor data is not lost if the recover timeout fails
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #18123:
URL: https://github.com/apache/pulsar/pull/18123#issuecomment-1356624951
The pr had no activity for 30 days, mark with Stale label.
--
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] poorbarcode commented on a diff in pull request #18123: [test][ml]Add test:Ensure that cursor data is not lost if the recover timeout fails and the retry succeeds
Posted by GitBox <gi...@apache.org>.
poorbarcode commented on code in PR #18123:
URL: https://github.com/apache/pulsar/pull/18123#discussion_r1000713957
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorImpl.java:
##########
@@ -500,6 +502,15 @@ protected void recoverFromLedger(final ManagedCursorInfo info, final VoidCallbac
if (log.isInfoEnabled()) {
log.info("[{}] Opened ledger {} for consumer {}. rc={}", ledger.getName(), ledgerId, name, rc);
}
+ // If ledgerMeta exists, it means ledger is not deleted normally and bookie is trying to recover the data.
+ // We will make several attempts to ensure that do not lose cursor data.
+ if (maybeLedgerRecovering(rc) && retryTimesWhenLedgerRecovering > 0) {
+ log.warn("[{}] Opened ledger {} for consumer {}. rc={}. Ledger maybe is recovering,"
+ + " try to recover again, at most try {} times", ledger.getName(), ledgerId, name, rc,
+ retryTimesWhenLedgerRecovering - 1);
+ recoverFromLedger(info, callback, retryTimesWhenLedgerRecovering - 1);
+ return;
+ }
Review Comment:
Hi @codelipenghui @mattisonchao
> I think treating the TimeoutException as a recoverable error is simple here. We are not able to load the data from the bookkeeper now. Just let the topic load get failed. The client side will trigger the topic loading again.
Good idea. I have submit a PR to make the timeout ex use a separate error code
--
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] poorbarcode commented on pull request #18123: [test][ml]Add test to ensure that cursor data is not lost if the recover timeout fails
Posted by GitBox <gi...@apache.org>.
poorbarcode commented on PR #18123:
URL: https://github.com/apache/pulsar/pull/18123#issuecomment-1289921866
/pulsarbot rerun-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