You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by "poorbarcode (via GitHub)" <gi...@apache.org> on 2025/01/02 11:00:25 UTC
[PR] [fix] [ml] fix key_shared mode delivery are order out order after a consumers reconnection [pulsar]
poorbarcode opened a new pull request, #23803:
URL: https://github.com/apache/pulsar/pull/23803
### Motivation
Please review https://github.com/apache/pulsar/pull/23802 first, after the PR merged, we start the current PR.
The case that https://github.com/apache/pulsar/pull/23802 described may also lead to delivery out of order.
TODO explains more details.
### Modifications
- Fix the issue.
- TODO: try to add a test to reproduce the issue
### 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 -->
- [ ] `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: x
--
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] [ml] fix key_shared mode delivery are order out order after a consumers reconnection [pulsar]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on PR #23803:
URL: https://github.com/apache/pulsar/pull/23803#issuecomment-2568976008
Please update the PR title. It currently includes `[ml]` and this PR doesn't seem to have anything to do with the managed ledger module.
One details about PR titles: don't add a space between the prefixes. instead of of `[fix] [broker]` use `[fix][broker]` prefix.
--
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] [ml] fix key_shared mode delivery are order out order after a consumers reconnection [pulsar]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #23803:
URL: https://github.com/apache/pulsar/pull/23803#issuecomment-2567596977
@poorbarcode Please add the following content to your PR description and select a checkbox:
```
- [ ] `doc` <!-- Your PR contains doc changes -->
- [ ] `doc-required` <!-- Your PR changes impact docs and you will update later -->
- [ ] `doc-not-needed` <!-- Your PR changes do not impact docs -->
- [ ] `doc-complete` <!-- Docs have been already added -->
```
--
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] [ml] fix key_shared mode delivery are order out order after a consumers reconnection [pulsar]
Posted by "poorbarcode (via GitHub)" <gi...@apache.org>.
poorbarcode commented on PR #23803:
URL: https://github.com/apache/pulsar/pull/23803#issuecomment-2609347441
@lhotari
> @poorbarcode This diff of this PR is currently messed up, so hard to see what the changes are.
Rebased master, it is clear now.
> Just wondering if the correct solution would be to prevent replay reads and normal reads to happen at the same time? I reverted such changes from the recent PR with this commit https://github.com/apache/pulsar/commit/06827df01022d656345386a8b9cc9ee6424257ae .
- A normal reading can not start when there is a pending reading
- A pending reading can start when there is a normal reading because maybe there are no more entries to read(the normal reading is just pending there and waiting for new messages)
I will modify the Motivation to make every thing clear late
--
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] [ml] fix key_shared mode delivery are order out order after a consumers reconnection [pulsar]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on PR #23803:
URL: https://github.com/apache/pulsar/pull/23803#issuecomment-2610019506
> * It may lead E2E latency increases
There would be an opposite impact when handled correctly. When there's a race between a pending read and a replay read, either one will be discarded. This is why extra work is performed and that leads to increased latencies eventually since unnecessary reads are performed. There's currently no broker side caching for replay reads, so discarded replay reads will cause extra traffic between broker and bookies. To avoid the extra latency, when the pending read gets cancelled and once the replay read is complete, a new pending read should be issued so that new entries get handled immediately when they are available. I'll be doing this type of change to address #23845, which is one of the issues where a read doesn't continue when a read gets skipped.
> * The pending normal read will be looped to `set` and `cancel`
> * It may be a conflict with the feature `read ahead before all messages sent are acknowledged`
Please explain more about these items since I didn't catch the meaning.
> * (Highlight) your solution can not solve the issue that this PR solves, see more detail at Motivation
When I made the comment, there was no description for this PR, so I'll check the updated description.
--
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] [ml] fix key_shared mode delivery are order out order after a consumers reconnection [pulsar]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on PR #23803:
URL: https://github.com/apache/pulsar/pull/23803#issuecomment-2609354877
> * A replay reading can start when there is a pending normal reading because maybe there are no more entries to read(the normal reading is just pending there and waiting for new messages)
"the normal reading is just pending there and waiting for new messages". It's better to cancel it since it does cause later problems. There's code to handle it, but the unnecessary work could be completely avoided.
--
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] [ml] fix key_shared mode delivery are order out order after a consumers reconnection [pulsar]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on PR #23803:
URL: https://github.com/apache/pulsar/pull/23803#issuecomment-2609317591
@poorbarcode This diff of this PR is currently messed up, so hard to see what the changes are.
Just wondering if the correct solution would be to prevent replay reads and normal reads to happen at the same time? I reverted such changes from the recent PR with this commit https://github.com/apache/pulsar/pull/23874/commits/06827df01022d656345386a8b9cc9ee6424257ae .
There's currently a problem that when a replay read starts, it doesn't cancel a pending read which is waiting for new entries.
--
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] [ml] fix key_shared mode delivery are order out order after a consumers reconnection [pulsar]
Posted by "poorbarcode (via GitHub)" <gi...@apache.org>.
poorbarcode commented on PR #23803:
URL: https://github.com/apache/pulsar/pull/23803#issuecomment-2609907241
> "the normal reading is just pending there and waiting for new messages". It's better to cancel it since it does cause later problems. There's code to handle it, but the unnecessary work could be completely avoided.
- It may lead E2E latency increases
- The pending normal read will be looped to `set` and `cancel`
- (Highlight) seems your solution will not solve the issue what this PR solves
--
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] [ml] fix key_shared mode delivery are order out order after a consumers reconnection [pulsar]
Posted by "poorbarcode (via GitHub)" <gi...@apache.org>.
poorbarcode commented on PR #23803:
URL: https://github.com/apache/pulsar/pull/23803#issuecomment-2611397108
@lhotari
BTW, we can start a new email channel to talk about this.
>> It may lead E2E latency increases
> There would be an opposite impact when handled correctly.
I consider that the current implementation is the best solution
>> The pending normal read will be looped to set and cancel
>> It may be a conflict with the feature read ahead before all messages sent are acknowledged
> Please explain more about these items since I didn't catch the meaning.
The case Read Ahead:
- there is consumer stuck, which leads messages being pushed into redeliver-queue
- there is consumer not stuck, which need to read new messages
I think we should not cancel `pending normal read`.
--
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] [ml] fix key_shared mode delivery are order out order after a consumers reconnection [pulsar]
Posted by "merlimat (via GitHub)" <gi...@apache.org>.
merlimat commented on PR #23803:
URL: https://github.com/apache/pulsar/pull/23803#issuecomment-6348463336
Thanks @poorbarcode, and sorry this sat for so long. The race you described (a replay read pending when the last consumer disconnects gets dispatched, then the deferred rewind delivers the same entries again) is fixed on master by #27007: any read that completes while the rewind is pending is discarded, and the rewind only happens once no read is in flight. The classic dispatchers this PR also changed were removed in #26687. I'm closing this as superseded. If you see a case #27007 doesn't cover, please reopen or file an issue with a repro against master.
--
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] [ml] fix key_shared mode delivery are order out order after a consumers reconnection [pulsar]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on PR #23803:
URL: https://github.com/apache/pulsar/pull/23803#issuecomment-2574869991
@poorbarcode [PIP-282](https://github.com/apache/pulsar/blob/master/pip/pip-282.md) (implementation PR #21953) was made to address known out-of-order issues in the Key_Shared implementation. PIP-282 was targeted for Pulsar 4.0 and later replaced with PIP-379 implementation. It feels that major changes to the Pulsar 3.0 Key_Shared implementation should be made carefully due to the high chance of changes causing other regressions.
This PR doesn't provide a test that reproduces the issue. Just wondering why we should keep on making major changes to Pulsar 3.0 Key_Shared implementation due to the risk of regressions. It would be better that customers move on to Pulsar 4.0 and the PIP-379 implementation where many of the problems have been addressed.
--
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] [ml] fix key_shared mode delivery are order out order after a consumers reconnection [pulsar]
Posted by "merlimat (via GitHub)" <gi...@apache.org>.
merlimat closed pull request #23803: [fix] [ml] fix key_shared mode delivery are order out order after a consumers reconnection
URL: https://github.com/apache/pulsar/pull/23803
--
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