Skip to content

3.x: constrain upstream requests from take, remove limit operator #6650

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Sep 13, 2019

Conversation

davidmoten
Copy link
Collaborator

As per discussion in #6569, this PR constrains upstream requests from the take operator and removes the limit operator.

I've also added a couple of unit tests for coverage of elementAt that I didn't include in #6620.

@codecov
Copy link

codecov bot commented Sep 12, 2019

Codecov Report

Merging #6650 into 3.x will decrease coverage by 0.03%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##                3.x    #6650      +/-   ##
============================================
- Coverage     98.11%   98.08%   -0.04%     
+ Complexity     6191     6186       -5     
============================================
  Files           678      677       -1     
  Lines         44722    44674      -48     
  Branches       6176     6166      -10     
============================================
- Hits          43880    43818      -62     
- Misses          294      302       +8     
- Partials        548      554       +6
Impacted Files Coverage Δ Complexity Δ
.../main/java/io/reactivex/rxjava3/core/Flowable.java 100% <ø> (ø) 559 <0> (-2) ⬇️
...ava3/internal/operators/flowable/FlowableTake.java 100% <100%> (ø) 2 <1> (ø) ⬇️
...l/operators/observable/ObservableFlatMapMaybe.java 83.8% <0%> (-4.93%) 2% <0%> (ø)
...al/operators/flowable/FlowableMergeWithSingle.java 97.59% <0%> (-2.41%) 2% <0%> (ø)
...nal/operators/flowable/FlowableMergeWithMaybe.java 97.66% <0%> (-2.34%) 2% <0%> (ø)
...va3/internal/operators/flowable/FlowableRange.java 95.87% <0%> (-2.07%) 3% <0%> (ø)
.../internal/disposables/ListCompositeDisposable.java 98% <0%> (-2%) 34% <0%> (-1%)
...ternal/operators/observable/ObservablePublish.java 97.29% <0%> (-1.81%) 16% <0%> (-1%)
...nal/operators/flowable/FlowableConcatMapEager.java 97.07% <0%> (-1.76%) 2% <0%> (ø)
...ava3/internal/operators/maybe/MaybeMergeArray.java 96.04% <0%> (-1.7%) 6% <0%> (ø)
... and 30 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 852e052...426c817. Read the comment docs.

@akarnokd akarnokd added this to the 3.0 milestone Sep 12, 2019
if (!get() && compareAndSet(false, true)) {
if (n >= limit) {
upstream.request(Long.MAX_VALUE);
while (true) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not use the limit() internals?

* Limits both the number of upstream items (after which the sequence completes)
* and the total downstream request amount requested from the upstream to
* possibly prevent the creation of excess items by the upstream.
* <p>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Javadoc of take should contain these extra information.

@akarnokd
Copy link
Member

Why not simply delete take and rename limit? That way, the tests and documentation of limit would remain intact.

@davidmoten
Copy link
Collaborator Author

Indeed! Will do.

@davidmoten davidmoten force-pushed the take-constrain-requests branch from c2d9279 to c499746 Compare September 13, 2019 03:13
@davidmoten davidmoten force-pushed the take-constrain-requests branch from c499746 to 426c817 Compare September 13, 2019 03:25
@davidmoten
Copy link
Collaborator Author

Copied code from limit to take and fixed javadoc.

@akarnokd akarnokd merged commit 0c50f0a into ReactiveX:3.x Sep 13, 2019
@davidmoten davidmoten deleted the take-constrain-requests branch September 13, 2019 21:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants