-
Notifications
You must be signed in to change notification settings - Fork 129
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
Context support #776
Context support #776
Conversation
|
return new Context(requireNonNull(entries, "The entries map cannot be null")); | ||
} | ||
|
||
private volatile ConcurrentHashMap<String, Object> entries; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you really need them to be both volatile
and concurrent?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In theory we could have 2 concurrent put
that'd cause the initialisation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I understand the need with the current design; but I'm curious about
- why the whole Context needs to be concurrent-safe (even for initialization and writes)
- why you opted for lazy initialization of the underlying map (I do love the attention towards allocations but I wonder how likely this is to not be allocated - and if it's very low, getting rid of this could simplify such code)
- did you consider immutable contexts? Mutations could be modelled as a new view over an existing context (admittedly this would be less flexible and only suitable for relatively small collections but it could be fine - so it's a deeper question about intended use)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Since you have 1
Context
instance for a whole subscription lifespan, there are cases where events and subscriptions get offset to an executor. Or you merge pipelines (e.g., 2 concurrent HTTP requests), etc. There is 0 guarantee we won't have concurrent access for such kind of object, hence IMHO it's better to make it concurrent-proof from the start. - The code is more elegant / safer with non-
null
Context
objects around, for instance some subscribers provide an empty context rather than flying anull
around. Deferring the allocation is indeed all about reducing the footprint: subscription that use a context will allocate, those who don't will not pay the full price. - My first iterations had been around read / write views, but I found the developer experience to be worse than it should be. First off immutable contexts are nice on the paper, but you need to remember that the binding methods are called at subscription time, which is counter-intuitive (events flow from publishers to subscribers, but here contexts flow from subscribers to publishers). I found out that have
withContext
(andattachContext
as sugar) to bind was simple and flexible (no need to patch tons of methods) and has clear semantics. Then I found out that there was little value in read/write views, if you are after manipulating a context across a whole pipeline then making it a shared, thread-safe object was more intuitive.
(hope it makes sense, lots of points in a single post 😄 )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Many thanks for all thourough explanations! Makes sense.
Marking as ready for review, but in any case let's have this PR bake a little bit. |
bdfdfac
to
095a457
Compare
Added Context context = Context.of("foo", "bar");
AssertSubscriber<String> sub = Multi.createFrom().context(ctx -> Multi.createBy()
.repeating().uni(
AtomicInteger::new,
counter -> Uni.createFrom()
.item(counter.incrementAndGet() + "::" + ctx.getOrElse("foo", () -> "yolo")))
.atMost(5))
.subscribe().withSubscriber(AssertSubscriber.create(context, Long.MAX_VALUE));
sub.assertCompleted();
assertThat(sub.getItems())
.hasSize(5)
.contains("1::bar", "5::bar"); and: Context context = Context.of("foo", "bar");
Multi<String> a = Multi.createFrom()
.context(ctx -> Multi.createFrom().items("a", ctx.getOrElse("foo", () -> "yolo")));
Multi<String> b = Multi.createFrom()
.context(ctx -> Multi.createFrom().items("b", ctx.getOrElse("foo", () -> "yolo")));
Multi<String> c = Multi.createFrom()
.context(ctx -> Multi.createFrom().items("c", ctx.getOrElse("foo", () -> "yolo")));
AssertSubscriber<String> sub = Multi.createBy().merging().streams(a, b, c)
.subscribe().withSubscriber(AssertSubscriber.create(context, Long.MAX_VALUE));
sub.assertCompleted();
assertThat(sub.getItems())
.hasSize(6)
.containsExactly("a", "bar", "b", "bar", "c", "bar"); |
Codecov Report
@@ Coverage Diff @@
## main #776 +/- ##
============================================
+ Coverage 89.10% 89.13% +0.02%
- Complexity 2987 3081 +94
============================================
Files 377 384 +7
Lines 11891 12161 +270
Branches 1500 1525 +25
============================================
+ Hits 10596 10840 +244
- Misses 682 700 +18
- Partials 613 621 +8
|
Codecov reports are wrong (it reports untested code that's actually being tested). |
90c15db
to
bc50085
Compare
bc50085
to
4cfc3af
Compare
@cescoffier Fancy a review to celebrate 2022? 😉 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM,
We would need feedback on the usage (so, this feature will be marked as experimental for a bit).
@cescoffier No remark on docs / etc? Yes all new APIs have been marked as experimental. |
No, it looks ok. However, I would need to try it to be sure it does what I believe it does :-D Typically, I'm not totally sure it could be used for the Multi serialization use case we have in reactive routes, where the Publisher must extend the context and this context would be read in the subscriber (to apply the right serialization). |
Typically, I'm not totally sure it could be used for the Multi serialization use case we have in reactive routes, where the Publisher must extend the context and this context would be read in the subscriber (to apply the right serialization).
Let me know if you need some delay before we merge this one.
|
Last call for comments, or I'll merge tomorrow (Tuesday 4th) morning my time 😃 |
...and of course I need to rebase and fix a conflict. |
This brings subscriber-provided contexts to Uni and Multi. Context can be materialized using the withContext and attachContext operators. See #757 This includes a squashed commit based on earlier explorations in https://github.com/jponge/smallrye-mutiny/tree/feature/context
4cfc3af
to
f02bc50
Compare
Go ahead!
… On 3 Jan 2022, at 20:24, Julien Ponge ***@***.***> wrote:
Last call for comments, or I'll merge tomorrow (Tuesday 4th) morning my time 😃
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you were mentioned.
|
It’s available in Mutiny 1.3.0
… On 13 Jan 2022, at 12:43, Guy Assaf ***@***.***> wrote:
@jponge <https://github.com/jponge> How can I consume it in my application, is it available as Maven dependency?
—
Reply to this email directly, view it on GitHub <#776 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAGK2M4VZSGDZZXZUXEK3DUV23GHANCNFSM5J7G2JTA>.
You are receiving this because you were mentioned.
|
Thanks @jponge, found it, an amazing job. I don't understand how to use the context, In this example, I debug and the code start from "number 3 " last reactive chain, why?
|
The Instead of manipulating the context from inside the lambda, you should perform the manipulations from an operator, as in: .withContext((someEntity, context) ->
someEntity.onItem().invoke(() -> LOGGER.info("number 3 UUID=" + context.get("UUID")) ) (not sure it compiles, it's a sketch) Does that make sense? |
So where do you manipulate (PUT) the object into the Context? We can move this conversation to a better place if this is not the right location. |
You can perform In general you should pass all context data that you know at subscription time (hint: I have no idea what happens in your case) and then you can read/write/delete in the context-aware portions of your pipeline. If you have further questions you can start a discussion at https://github.com/smallrye/smallrye-mutiny/discussions |
Thanks I will open a discussion. |
This brings subscriber-provided contexts to
Uni
andMulti
.Context can be materialized using the
withContext
andattachContext
operators.See #757
This is a squashed commit based on explorations in https://github.com/jponge/smallrye-mutiny/tree/feature/context