Skip to content
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

refactor: multi subscriber adapter refactorings and fixes #1516

Merged

Conversation

jponge
Copy link
Member

@jponge jponge commented Feb 15, 2024

No description provided.

…indowOp

This was hidden in previous Mutiny releases as test ran with StrictMultiSubscriber
between upstreams and AssertSubscriber.

The operators would perform an overflow on negative requests.
This is cosmetic, but in theory a subscriber could request -1 then 5 elements,
and then still receive onItem(n) signals.
The TCK randomly _and_ wrongly flags violations of rule 1.3 with the first onNext() signal
happening while onSubscribe() hasn't finished yet.
This is especially useful for the operators that manage
local demand.
Removes the cost of StrictMultiSubscriber when plugging non-Mutiny subscribers.
This introduces MultiSubscriberAdapter, a very thin forwarding adapter between
Flow.Subscriber and MultiSubscriber + ContextSupport.

Note that StrictMultiSubscriber handled some pieces of the Reactive Streams protocol
such as bad request(n) calls, upstream operators are not shielded on incomplete
implementations anymore.
@jponge jponge added bug Something isn't working enhancement New feature or request technical-debt Technical debt reduction internal Some internal enhancement labels Feb 15, 2024
@jponge jponge added this to the 2.6.0 milestone Feb 15, 2024
@jponge
Copy link
Member Author

jponge commented Feb 15, 2024

@ozangunalp if you want to test against Reactive Messaging you will need https://github.com/jponge/smallrye-reactive-messaging/tree/scratchpad/mutiny-queues-refactor

@ozangunalp
Copy link
Collaborator

Yes I've that change from testing the jctools PR.
But this one seems slightly more complicated :)

Copy link

codecov bot commented Feb 15, 2024

Codecov Report

Attention: 26 lines in your changes are missing coverage. Please review.

Comparison is base (d72c784) 89.23% compared to head (3c6b3f4) 89.43%.
Report is 11 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##               main    #1516      +/-   ##
============================================
+ Coverage     89.23%   89.43%   +0.20%     
- Complexity     3270     3282      +12     
============================================
  Files           456      457       +1     
  Lines         13200    13264      +64     
  Branches       1622     1637      +15     
============================================
+ Hits          11779    11863      +84     
+ Misses          794      788       -6     
+ Partials        627      613      -14     
Files Coverage Δ
...rye/mutiny/converters/uni/UniToMultiPublisher.java 90.24% <100.00%> (+2.43%) ⬆️
...smallrye/mutiny/helpers/MultiEmitterProcessor.java 90.56% <100.00%> (+0.36%) ⬆️
...va/io/smallrye/mutiny/operators/AbstractMulti.java 100.00% <100.00%> (ø)
...smallrye/mutiny/operators/multi/MultiBufferOp.java 89.47% <100.00%> (+0.76%) ⬆️
.../smallrye/mutiny/operators/multi/MultiCacheOp.java 89.65% <100.00%> (+6.12%) ⬆️
...rye/mutiny/operators/multi/MultiDemandCapping.java 93.33% <100.00%> (ø)
...rye/mutiny/operators/multi/MultiOnRequestCall.java 96.42% <100.00%> (+6.10%) ⬆️
...mutiny/operators/multi/MultiOperatorProcessor.java 92.72% <100.00%> (+5.45%) ⬆️
...rye/mutiny/operators/multi/MultiSelectFirstOp.java 89.18% <100.00%> (-0.82%) ⬇️
...lrye/mutiny/operators/multi/MultiSelectLastOp.java 84.74% <100.00%> (ø)
... and 25 more

... and 6 files with indirect coverage changes

@jponge
Copy link
Member Author

jponge commented Feb 15, 2024

It should be quite transparent actually (famous last words 🤣 )

@jponge
Copy link
Member Author

jponge commented Feb 15, 2024

I might add a commit to address diff coverage

@ozangunalp
Copy link
Collaborator

@ozangunalp if you want to test against Reactive Messaging you will need https://github.com/jponge/smallrye-reactive-messaging/tree/scratchpad/mutiny-queues-refactor

Haven't checked the code but Reactive Messaging tests are OK.

@jponge
Copy link
Member Author

jponge commented Feb 19, 2024

@ozangunalp if you want to test against Reactive Messaging you will need https://github.com/jponge/smallrye-reactive-messaging/tree/scratchpad/mutiny-queues-refactor

Haven't checked the code but Reactive Messaging tests are OK.

Thanks Ozan. A pass on the code would be appreciated, but only if you have time, otherwise I'll go ahead and merge

@jponge jponge merged commit cf2c85f into smallrye:main Feb 19, 2024
6 of 7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request internal Some internal enhancement technical-debt Technical debt reduction
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants