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

fix: rewrite the concatMap operator #1489

Merged
merged 1 commit into from Jan 18, 2024
Merged

fix: rewrite the concatMap operator #1489

merged 1 commit into from Jan 18, 2024

Conversation

jponge
Copy link
Member

@jponge jponge commented Jan 16, 2024

In this implementation the operator acts as a simple no-queue forwarder
between the subscriber and the current upstream.

There is also a small dose of fine-grained locking around a few
state machine updates that can’t be expressed as non-blocking / compare & swap
operations.

@jponge
Copy link
Member Author

jponge commented Jan 16, 2024

@ozangunalp How does this sound against the main branch of Reactive Messaging?

Copy link

codecov bot commented Jan 16, 2024

Codecov Report

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

Comparison is base (82adadc) 89.23% compared to head (dded2d6) 89.34%.
Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##               main    #1489      +/-   ##
============================================
+ Coverage     89.23%   89.34%   +0.10%     
- Complexity     3362     3365       +3     
============================================
  Files           459      459              
  Lines         13405    13434      +29     
  Branches       1642     1635       -7     
============================================
+ Hits          11962    12002      +40     
- Misses          805      808       +3     
+ Partials        638      624      -14     
Files Coverage Δ
...in/java/io/smallrye/mutiny/groups/MultiOnItem.java 100.00% <100.00%> (ø)
...llrye/mutiny/operators/multi/MultiConcatMapOp.java 87.33% <87.80%> (+4.93%) ⬆️

... and 14 files with indirect coverage changes

@ozangunalp
Copy link
Collaborator

@jponge Reactive Messaging tests are green 🥇
Looking at the code now

@ozangunalp
Copy link
Collaborator

It seems all correct to me.
Only weird thing to me is some updates to the state are done with CAS field updater others with the lock.

@jponge
Copy link
Member Author

jponge commented Jan 17, 2024

It seems all correct to me. Only weird thing to me is some updates to the state are done with CAS field updater others with the lock.

Thanks Ozan!

Yes some updates can be done in a CAS, but in some cases you need to read the current state but update depending on some other condition (e.g., is there any outstanding demand or not), which IMHO can't be expressed as a CAS.

@ozangunalp
Copy link
Collaborator

but in some cases you need to read the current state but update depending on some other condition

Exactly that!

In this implementation the operator acts as a simple no-queue forwarder
between the subscriber and the current upstream.

There is also a small dose of fine-grained locking around a few
state machine updates that can’t be expressed as non-blocking / compare & swap
operations.
@jponge jponge marked this pull request as ready for review January 17, 2024 21:52
@jponge jponge added the bug Something isn't working label Jan 17, 2024
@jponge jponge added this to the 2.5.4 milestone Jan 17, 2024
@jponge
Copy link
Member Author

jponge commented Jan 17, 2024

@ozangunalp May I ask you one more run? (I did a few edits, nothing that should break, but those are always famous last words)

@ozangunalp
Copy link
Collaborator

I've checked again. Reactive messaging tests in local are green.

@cescoffier
Copy link
Contributor

didn't notice anything weird.

@jponge
Copy link
Member Author

jponge commented Jan 18, 2024

didn't notice anything weird.

... "Yet" 🤣

@jponge jponge merged commit 0adf52a into main Jan 18, 2024
7 checks passed
@jponge jponge deleted the fix/concatmap-rewrite branch January 18, 2024 14:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants