[fix][broker] Revert "[fix][broker] Cancel possible pending replay read in cancelPendingRead (#23384)"

[lhotari]

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 [fix][broker] Cancel possible pending replay read in cancelPendingRead #23384 changes
• revert also this in PersistentDispatcherMultipleConsumersClassic
• add javadoc

Documentation

• doc
• doc-required
• doc-not-needed
• doc-complete

[poorbarcode]

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?

[lhotari]

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?

@poorbarcode 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.

Explained in comment #23855 (comment)

[codecov-commenter]

Codecov 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) to head (41ad1ff).
Report is 854 commits behind head on master.

Files with missing lines	Patch %	Lines
.../PersistentDispatcherMultipleConsumersClassic.java	0.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             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	Coverage Δ
inttests	26.66% <0.00%> (+2.08%)	⬆️
systests	23.23% <0.00%> (-1.09%)	⬇️
unittests	73.70% <50.00%> (+0.85%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
...r/service/AbstractDispatcherMultipleConsumers.java	83.60% <ø> (+1.34%)	⬆️
...sistent/PersistentDispatcherMultipleConsumers.java	74.58% <100.00%> (+0.25%)	⬆️
.../PersistentDispatcherMultipleConsumersClassic.java	44.72% <0.00%> (ø)

... and 1025 files with indirect coverage changes

[poorbarcode]

@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

[lhotari]

@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 #23803, it does not matter, could you add a test for the current PR to avoid others doing the same change as #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.

[poorbarcode]

@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

[lhotari]

@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.