You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by "BewareMyPower (via GitHub)" <gi...@apache.org> on 2024/03/05 05:58:22 UTC
[PR] [fix][client] fix Reader.hasMessageAvailable might return true after seeking to latest [pulsar]
BewareMyPower opened a new pull request, #22201:
URL: https://github.com/apache/pulsar/pull/22201
### Motivation
Java client has the same issue with
https://github.com/apache/pulsar-client-python/issues/199
After a seek operation is done, the `startMessageId` will be updated until the reconnection due to the seek is done in `connectionOpened`. So before it's updated, `hasMessageAvailable` could compare with an outdated `startMessageId` and return a wrong value.
### Modifications
Replace `duringSeek` with a `SeekStatus` field:
- `NOT_STARTED`: initial, or a seek operation is done. `seek` could only succeed in this status.
- `IN_PROGRESS`: A seek operation has started but the client does not receive the response from broker.
- `COMPLETED`: The client has received the seek response but the seek future is not done.
After the status becomes `COMPLETED`, next time the connection is established, the status will change from `COMPLETED` to `NOT_STARTED` and then seek future will be completed in the internal executor.
Add `testHasMessageAvailableAfterSeek` to cover this change.
### 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:
--
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][client] Fix Reader.hasMessageAvailable might return true after seeking to latest [pulsar]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on PR #22201:
URL: https://github.com/apache/pulsar/pull/22201#issuecomment-2024096913
It's necessary to cherry-pick #20963 so that this fix could be applied cleanly.
--
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][client] fix Reader.hasMessageAvailable might return true after seeking to latest [pulsar]
Posted by "BewareMyPower (via GitHub)" <gi...@apache.org>.
BewareMyPower commented on code in PR #22201:
URL: https://github.com/apache/pulsar/pull/22201#discussion_r1513919792
##########
pulsar-client/src/main/java/org/apache/pulsar/client/impl/ConsumerImpl.java:
##########
@@ -2279,14 +2287,25 @@ private void seekAsyncInternal(long requestId, ByteBuf seek, MessageId seekId, S
lastDequeuedMessageId = MessageId.earliest;
clearIncomingMessages();
- seekFuture.complete(null);
+ CompletableFuture<Void> future = null;
+ synchronized (this) {
+ if (!hasParentConsumer && cnx() == null) {
Review Comment:
I opened an issue: https://github.com/apache/pulsar/issues/22208
It's not related to this PR but an existing test could be affected by this PR so I got it around.
--
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][client] fix Reader.hasMessageAvailable might return true after seeking to latest [pulsar]
Posted by "BewareMyPower (via GitHub)" <gi...@apache.org>.
BewareMyPower commented on code in PR #22201:
URL: https://github.com/apache/pulsar/pull/22201#discussion_r1512907076
##########
pulsar-client/src/main/java/org/apache/pulsar/client/impl/ConsumerImpl.java:
##########
@@ -2279,14 +2287,25 @@ private void seekAsyncInternal(long requestId, ByteBuf seek, MessageId seekId, S
lastDequeuedMessageId = MessageId.earliest;
clearIncomingMessages();
- seekFuture.complete(null);
+ CompletableFuture<Void> future = null;
+ synchronized (this) {
+ if (!hasParentConsumer && cnx() == null) {
Review Comment:
Disable the behavior to fix `testSeekCustomTopicMessageId`.
Things become complicated when taking care of seeking an internal consumer of a multi-topics consumer.
1. All incoming messages are cleared.
2. Consumer A seeks to a given position
3. All incoming messages are cleared.
4. Consumer B seeks to a given position. **At the same time, consumer A is receiving messages.**
5. All incoming messages are cleared, including the messages received by consumer A.
6. Consumer C seeks to a given position.
7. Call `receive` and the cleared messages of consumer A could never be received.
So the messages of consumer A cannot be received after all these seek operations are done. I think it's an issue with the `MultiTopicsConsumer#seek` so I just get it around in this PR. The race condition should also exist for the previous code. This PR adds more latency to `ConsumerImpl#seek` so the issue is exposed.
--
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][client] fix Reader.hasMessageAvailable might return true after seeking to latest [pulsar]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #22201:
URL: https://github.com/apache/pulsar/pull/22201#issuecomment-1980232160
## [Codecov](https://app.codecov.io/gh/apache/pulsar/pull/22201?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
Attention: Patch coverage is `80.00000%` with `9 lines` in your changes are missing coverage. Please review.
> Project coverage is 73.56%. Comparing base [(`bbc6224`)](https://app.codecov.io/gh/apache/pulsar/commit/bbc62245c5ddba1de4b1e7cee4ab49334bc36277?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`901c6a9`)](https://app.codecov.io/gh/apache/pulsar/pull/22201?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
> Report is 23 commits behind head on master.
<details><summary>Additional details and impacted files</summary>
[](https://app.codecov.io/gh/apache/pulsar/pull/22201?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
```diff
@@ Coverage Diff @@
## master #22201 +/- ##
============================================
- Coverage 73.57% 73.56% -0.01%
+ Complexity 32624 32071 -553
============================================
Files 1877 1878 +1
Lines 139502 139663 +161
Branches 15299 15335 +36
============================================
+ Hits 102638 102750 +112
- Misses 28908 28931 +23
- Partials 7956 7982 +26
```
| [Flag](https://app.codecov.io/gh/apache/pulsar/pull/22201/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [inttests](https://app.codecov.io/gh/apache/pulsar/pull/22201/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `26.69% <48.88%> (+2.11%)` | :arrow_up: |
| [systests](https://app.codecov.io/gh/apache/pulsar/pull/22201/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `24.30% <48.88%> (-0.03%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pulsar/pull/22201/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `72.82% <80.00%> (-0.02%)` | :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=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Files](https://app.codecov.io/gh/apache/pulsar/pull/22201?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [...va/org/apache/pulsar/client/impl/ConsumerImpl.java](https://app.codecov.io/gh/apache/pulsar/pull/22201?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2NsaWVudC9pbXBsL0NvbnN1bWVySW1wbC5qYXZh) | `77.50% <80.00%> (-0.07%)` | :arrow_down: |
... and [68 files with indirect coverage changes](https://app.codecov.io/gh/apache/pulsar/pull/22201/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
</details>
--
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][client] fix Reader.hasMessageAvailable might return true after seeking to latest [pulsar]
Posted by "codelipenghui (via GitHub)" <gi...@apache.org>.
codelipenghui merged PR #22201:
URL: https://github.com/apache/pulsar/pull/22201
--
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][client] fix Reader.hasMessageAvailable might return true after seeking to latest [pulsar]
Posted by "BewareMyPower (via GitHub)" <gi...@apache.org>.
BewareMyPower commented on PR #22201:
URL: https://github.com/apache/pulsar/pull/22201#issuecomment-1978539909
There seems to be something wrong for the multi-topics consumer seek. I'm investigating
--
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