NOTIF-55 Use Multi when relevant, replace with Uni<List> otherwise #235
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
gwenneg
commented
May 10, 2021
| * - database queries are executed in an asynchronous way using `.onItem().call(...)` | ||
| * - side-effects are executed in a synchronous way using `.onItem().invoke(...)` | ||
| */ | ||
| return Uni.createFrom().item(() -> new SettingsValues()) |
There was a problem hiding this comment.
This is roughly the same code than before but we're no longer using Multi in this method.
gwenneg
force-pushed
the
replace-multi-with-uni
branch
from
ab72e11 to
65e698b
Compare
gwenneg
force-pushed
the
replace-multi-with-uni
branch
2 times, most recently
from
f891ac3 to
d5d589a
Compare
|
Rebased and fixed conflicts. |
sorry, merged something and there is one conflict now |
josejulio
approved these changes
May 12, 2021
gwenneg
force-pushed
the
replace-multi-with-uni
branch
from
d5d589a to
de2b789
Compare
|
I solved the conflict. Thanks for the review! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is preparation work for #202.
I found something about the response time issue with
quarkus-resteasy-reactive: the response time can be bad only with APIs that returnMulti. There's no problem with methods returningUni<List>, which should always be the type we use anyway because we never need to stream data back to the client. I created quarkusio/quarkus#17094 to report the issue withMultito the Quarkus team.Since we're using
Multiin many places for wrong reasons, I did some spring cleaning and replacedMultiwithUni<List>. A few methods could be cleaned further but I chose not to because they will be removed with phase 2 of behavior groups.Here are some ground rules for future developments:
*Resourcesclasses) should not returnMultiunless there's a very good reason to do so (usually, there's not).*Serviceclasses) should not returnMultiunless we need to stream data back to the client (that's very unlikely in this app).Multito transform items fromUni<List>. There are situations when it makes sense but most of the time it doesn't. In particular, side-effects should be implemented withonItem().invoke()(synchronous, for code that will not block the I/O thread) oronItem().call()(asynchronous, for DB queries for example).