You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by "TakaHiR07 (via GitHub)" <gi...@apache.org> on 2023/11/10 03:36:21 UTC
[PR] [fix][broker] Fix topic compaction is failed after compactedLedger's all quorum is being recover [pulsar]
TakaHiR07 opened a new pull request, #21552:
URL: https://github.com/apache/pulsar/pull/21552
Fixes #21551
### Motivation
Fix topic compaction is failed after compactedLedger's all quorum is being recover, which is described in issue.
### Modifications
When CompactedTopicImpl try to open compactedLedger, use asyncOpenLedgerNoRecovery instead of asyncOpenLedger. Then compactedLedger can watch the zk node change and update the ledger metadata in broker.
### Does this pull request potentially affect one of the following parts:
<!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. -->
*If the box was checked, please highlight the changes*
- [ ] Dependencies (add or upgrade a dependency)
- [ ] The public API
- [ ] The schema
- [ ] The default values of configurations
- [ ] The threading model
- [ ] The binary protocol
- [ ] The REST endpoints
- [ ] The admin CLI options
- [ ] The metrics
- [ ] Anything that affects deployment
### Documentation
<!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. -->
- [ ] `doc` <!-- Your PR contains doc changes. -->
- [ ] `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: <!-- ENTER URL 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
Re: [PR] [fix][broker] Fix topic compaction is failed after compactedLedger's all quorum is being recover [pulsar]
Posted by "TakaHiR07 (via GitHub)" <gi...@apache.org>.
TakaHiR07 commented on PR #21552:
URL: https://github.com/apache/pulsar/pull/21552#issuecomment-2639028631
This problem has not been fixed and I hope to continue the review of this pr.
Firstly, I think the data inconsistent issue of asyncOpenLedgerNoRecovery is not exist.
- Because we can read the compactedLedger until ledger become Closed. There is no write client and read client exist at the same time. Just as the following code show, we close the ledger success and then do reader.acknowledgeCumulativeAsync, then COMPACTED_TOPIC_LEDGER_PROPERTY would be changed to the specific ledgerId, and then we asyncOpenLedgerNoRecovery the compactedLedger and the compactedTopicContext would be created.
- If the write client not finish, we can not create compactedTopicContext and can not create the read client.
https://github.com/apache/pulsar/blob/8a40b30cf47a91ec02d931e6371d02409ba5951e/pulsar-broker/src/main/java/org/apache/pulsar/compaction/AbstractTwoPhaseCompactor.java#L184-L212
Second, we only register listener for compactedLedger, the listener number is not huge.
@codelipenghui @zymap @codelipenghui @yebai1105 @coderzc
--
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] [fix][broker] Fix topic compaction is failed after compactedLedger's all quorum is being recover [pulsar]
Posted by "merlimat (via GitHub)" <gi...@apache.org>.
merlimat closed pull request #21552: [fix][broker] Fix topic compaction is failed after compactedLedger's all quorum is being recover
URL: https://github.com/apache/pulsar/pull/21552
--
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] [fix][broker] Fix topic compaction is failed after compactedLedger's all quorum is being recover [pulsar]
Posted by "yebai1105 (via GitHub)" <gi...@apache.org>.
yebai1105 commented on PR #21552:
URL: https://github.com/apache/pulsar/pull/21552#issuecomment-2097566320
I have the same problem, is there any new progress?
--
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] [fix][broker] Fix topic compaction is failed after compactedLedger's all quorum is being recover [pulsar]
Posted by "zymap (via GitHub)" <gi...@apache.org>.
zymap commented on PR #21552:
URL: https://github.com/apache/pulsar/pull/21552#issuecomment-1825061953
openLedger is a frequent operation, If every openLedger operation registers the listener, that would be a huge number.
If one broker can not communicate with zookeeper and doesn't close the compacted ledger, then a new broker opens the ledger. That would take some unexpected things. Can we find a way to make the ledgerhandle refreshed if all reads failed?
--
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] [fix][broker] Fix topic compaction is failed after compactedLedger's all quorum is being recover [pulsar]
Posted by "zymap (via GitHub)" <gi...@apache.org>.
zymap commented on PR #21552:
URL: https://github.com/apache/pulsar/pull/21552#issuecomment-1825047476
In Pulsar the bookkeeper client uses the pulsar metadata implementation, and It seems the broker's metadata cache has the old information and didn't update properly.
In Bookkeeper, it has the watcher, but it seems Pulsar's implementation didn't do the same thing. Maybe need to take a deep look.
--
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] [fix][broker] Fix topic compaction is failed after compactedLedger's all quorum is being recover [pulsar]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on PR #21552:
URL: https://github.com/apache/pulsar/pull/21552#issuecomment-3227324550
possible related: https://github.com/apache/bookkeeper/pull/4613
--
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] [fix][broker] Fix topic compaction is failed after compactedLedger's all quorum is being recover [pulsar]
Posted by "TakaHiR07 (via GitHub)" <gi...@apache.org>.
TakaHiR07 commented on PR #21552:
URL: https://github.com/apache/pulsar/pull/21552#issuecomment-1818210566
> As you described in the issue, it seems the ledger doesn't replicate successfully to the new ensemble. The metadata is still the old one:
>
> ```
> Read of ledger entry failed: L1705537 E0-E0, Sent to [bookie-1, bookie-2, bookie-3], Heard from [] : bitset = {}, Error = 'Bookie handle is not available'. First unread entry is (-1, rc = null)
> ```
>
> It is still trying to read the old ensembles.
>
> `asyncOpenLedgerNoRecovery` is not recommended because it could receive different responses on the different bookies. It violates the bookie's write consistent policy. For example, when you write 3 bookies and you need 2 bookie ack but only receive one ack. Using this API can read that entry from one bookie but can not read it from the another the failed write bookie. The write client know it's a failed write. But the read client can read it.
Replicate of auto-recovery is success, metadata on zk is the new one, but metadata on CompactTopicContext#ReadLedgerHandle is the old one. And I can read ledger directly from bookie-shell.
I guess compactedLedger is closed ledger, it should not be write and read at the same time. Therefore the read client should not be able to read the failed write entry ??
And I also have a question here. Why asyncOpenLedger not register the zk listener to update metadata change?
--
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] [fix][broker] Fix topic compaction is failed after compactedLedger's all quorum is being recover [pulsar]
Posted by "yebai1105 (via GitHub)" <gi...@apache.org>.
yebai1105 commented on PR #21552:
URL: https://github.com/apache/pulsar/pull/21552#issuecomment-2097565885
I have the same problem, is there any new progress?
--
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] [fix][broker] Fix topic compaction is failed after compactedLedger's all quorum is being recover [pulsar]
Posted by "merlimat (via GitHub)" <gi...@apache.org>.
merlimat commented on PR #21552:
URL: https://github.com/apache/pulsar/pull/21552#issuecomment-6353901610
Thanks @TakaHiR07 for chasing this down, and sorry it sat so long. The root cause you found — a recovery-opened handle never picking up the ensemble change from auto-recovery — was fixed in BookKeeper (apache/bookkeeper#4613, `keepUpdateMetadata`) and Pulsar now opens the compacted ledger with it since #25199 (in 4.0.9, 4.1.3 and 4.2). I'm closing this as superseded, along with #21551. If you still see compaction stuck on one of those versions, please reopen the issue with the broker logs.
--
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