You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by "lhotari (via GitHub)" <gi...@apache.org> on 2025/01/16 20:14:04 UTC
[PR] [fix][broker] Revert "[fix][broker] Cancel possible pending replay read in cancelPendingRead (#23384)" [pulsar]
lhotari opened a new pull request, #23855:
URL: https://github.com/apache/pulsar/pull/23855
### Motivation
There was a misunderstanding in making the change #23384. This PR reverts the change and adds javadoc to the `cancelPendingReadRequest` method to explain the purpose of the method.
### Modifications
- revert #23384 changes
- revert also this in PersistentDispatcherMultipleConsumersClassic
- add javadoc
### 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 -->
--
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] Revert "[fix][broker] Cancel possible pending replay read in cancelPendingRead (#23384)" [pulsar]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on PR #23855:
URL: https://github.com/apache/pulsar/pull/23855#issuecomment-2597518920
> @lhotari
>
> > I don't think that it's justified to block this PR with "request for changes". It's not a common practice to include tests for revert commits. We could add tests in other PRs.
>
> Sure, please create a new PR to add the test
> Seems I have mentioned this issue at https://github.com/apache/pulsar/pull/23803, it does not matter, could you add a test for the current PR to avoid others doing the same change as https://github.com/apache/pulsar/pull/23384?
@poorbarcode What type of test do you have in mind? In #23803 there aren't any comments about what you have in mind and there are no tests. Adding a test to avoid others doing the same change doesn't really make sense to me. A better way to address the problem would be to rename the method if we'd like to improve the clarity.
btw. I'm currently working on #23845 and it might result in adding some other tests in this area.
--
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] Revert "[fix][broker] Cancel possible pending replay read in cancelPendingRead (#23384)" [pulsar]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on PR #23855:
URL: https://github.com/apache/pulsar/pull/23855#issuecomment-2597414484
> Seems I have mentioned this issue at #23803, it does not matter, could you add a test for the current PR to avoid others doing the same change as #23384?
It's a common practice to revert a change that turned out to be invalid. In this case, although a similar change is made in #23384, it won't cause merge conflicts even if it were to be merged later. This PR contains a javadoc comment to improve the clarity of `cancelPendingRead`. I don't think that it's justified to block this PR with "request for changes". It's not a common practice to include tests for revert commits. We could add tests in other PRs.
--
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] Revert "[fix][broker] Cancel possible pending replay read in cancelPendingRead (#23384)" [pulsar]
Posted by "poorbarcode (via GitHub)" <gi...@apache.org>.
poorbarcode commented on PR #23855:
URL: https://github.com/apache/pulsar/pull/23855#issuecomment-2597505711
@lhotari
> I don't think that it's justified to block this PR with "request for changes". It's not a common practice to include tests for revert commits. We could add tests in other PRs.
Sure, please create a new PR to add the test
--
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] Revert "[fix][broker] Cancel possible pending replay read in cancelPendingRead (#23384)" [pulsar]
Posted by "poorbarcode (via GitHub)" <gi...@apache.org>.
poorbarcode merged PR #23855:
URL: https://github.com/apache/pulsar/pull/23855
--
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] Revert "[fix][broker] Cancel possible pending replay read in cancelPendingRead (#23384)" [pulsar]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on PR #23855:
URL: https://github.com/apache/pulsar/pull/23855#issuecomment-2597532250
> @lhotari
>
> > What type of test do you have in mind?
>
> A test to reproduce the issue that #23384 introduced, which can prevent others from making the same change as #23384
@poorbarcode There was no known issue that #23384 introduced. It's just a wrong change. For read requests, it's not possible to "cancel" a replay read and therefore having the logic in `cancelPendingReadRequest` method for setting `havePendingReplayRead=false` in the method doesn't simply make sense. I have clarified the meaning in the javadoc in this PR: "Cancel a possible pending read that is a Managed Cursor waiting to be notified for more entries. This won't cancel any other pending reads that are currently in progress.".
As I mentioned in the previous comment, renaming this method to a more specific name would be a better way to ensure that the abstraction doesn't get misunderstood in the future.
--
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] Revert "[fix][broker] Cancel possible pending replay read in cancelPendingRead (#23384)" [pulsar]
Posted by "poorbarcode (via GitHub)" <gi...@apache.org>.
poorbarcode commented on PR #23855:
URL: https://github.com/apache/pulsar/pull/23855#issuecomment-2597346047
Seems I have mentioned this issue at https://github.com/apache/pulsar/pull/23803, it does not matter, could you add a test for the current PR to avoid others doing the same change as https://github.com/apache/pulsar/pull/23384
--
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] Revert "[fix][broker] Cancel possible pending replay read in cancelPendingRead (#23384)" [pulsar]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #23855:
URL: https://github.com/apache/pulsar/pull/23855#issuecomment-2597471306
## [Codecov](https://app.codecov.io/gh/apache/pulsar/pull/23855?dropdown=coverage&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 `50.00000%` with `1 line` in your changes missing coverage. Please review.
> Project coverage is 74.17%. Comparing base [(`bbc6224`)](https://app.codecov.io/gh/apache/pulsar/commit/bbc62245c5ddba1de4b1e7cee4ab49334bc36277?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`41ad1ff`)](https://app.codecov.io/gh/apache/pulsar/commit/41ad1ff9866d858a32a1bc3d0b58a7a78cfbe5a1?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
> Report is 851 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/pulsar/pull/23855?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [.../PersistentDispatcherMultipleConsumersClassic.java](https://app.codecov.io/gh/apache/pulsar/pull/23855?src=pr&el=tree&filepath=pulsar-broker%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpulsar%2Fbroker%2Fservice%2Fpersistent%2FPersistentDispatcherMultipleConsumersClassic.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9zZXJ2aWNlL3BlcnNpc3RlbnQvUGVyc2lzdGVudERpc3BhdGNoZXJNdWx0aXBsZUNvbnN1bWVyc0NsYXNzaWMuamF2YQ==) | 0.00% | [0 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/pulsar/pull/23855?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
[](https://app.codecov.io/gh/apache/pulsar/pull/23855?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
```diff
@@ Coverage Diff @@
## master #23855 +/- ##
============================================
+ Coverage 73.57% 74.17% +0.60%
+ Complexity 32624 2376 -30248
============================================
Files 1877 1853 -24
Lines 139502 143596 +4094
Branches 15299 16306 +1007
============================================
+ Hits 102638 106519 +3881
+ Misses 28908 28663 -245
- Partials 7956 8414 +458
```
| [Flag](https://app.codecov.io/gh/apache/pulsar/pull/23855/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/23855/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `26.66% <0.00%> (+2.08%)` | :arrow_up: |
| [systests](https://app.codecov.io/gh/apache/pulsar/pull/23855/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `23.23% <0.00%> (-1.09%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pulsar/pull/23855/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `73.70% <50.00%> (+0.85%)` | :arrow_up: |
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 with missing lines](https://app.codecov.io/gh/apache/pulsar/pull/23855?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [...r/service/AbstractDispatcherMultipleConsumers.java](https://app.codecov.io/gh/apache/pulsar/pull/23855?src=pr&el=tree&filepath=pulsar-broker%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpulsar%2Fbroker%2Fservice%2FAbstractDispatcherMultipleConsumers.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9zZXJ2aWNlL0Fic3RyYWN0RGlzcGF0Y2hlck11bHRpcGxlQ29uc3VtZXJzLmphdmE=) | `83.60% <ø> (+1.34%)` | :arrow_up: |
| [...sistent/PersistentDispatcherMultipleConsumers.java](https://app.codecov.io/gh/apache/pulsar/pull/23855?src=pr&el=tree&filepath=pulsar-broker%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpulsar%2Fbroker%2Fservice%2Fpersistent%2FPersistentDispatcherMultipleConsumers.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9zZXJ2aWNlL3BlcnNpc3RlbnQvUGVyc2lzdGVudERpc3BhdGNoZXJNdWx0aXBsZUNvbnN1bWVycy5qYXZh) | `74.58% <100.00%> (+0.25%)` | :arrow_up: |
| [.../PersistentDispatcherMultipleConsumersClassic.java](https://app.codecov.io/gh/apache/pulsar/pull/23855?src=pr&el=tree&filepath=pulsar-broker%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpulsar%2Fbroker%2Fservice%2Fpersistent%2FPersistentDispatcherMultipleConsumersClassic.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9zZXJ2aWNlL3BlcnNpc3RlbnQvUGVyc2lzdGVudERpc3BhdGNoZXJNdWx0aXBsZUNvbnN1bWVyc0NsYXNzaWMuamF2YQ==) | `44.72% <0.00%> (ø)` | |
... and [1025 files with indirect coverage changes](https://app.codecov.io/gh/apache/pulsar/pull/23855/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][broker] Revert "[fix][broker] Cancel possible pending replay read in cancelPendingRead (#23384)" [pulsar]
Posted by "poorbarcode (via GitHub)" <gi...@apache.org>.
poorbarcode commented on PR #23855:
URL: https://github.com/apache/pulsar/pull/23855#issuecomment-2597522865
@lhotari
> What type of test do you have in mind?
A test to reproduce the issue that #23384 introduced, which can prevent others from making the same change as https://github.com/apache/pulsar/pull/23384
--
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