-
-
Notifications
You must be signed in to change notification settings - Fork 320
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
Add shared stream interfaces #1449
Add shared stream interfaces #1449
Conversation
Signed-off-by: Matei David <[email protected]>
Signed-off-by: Matei David <[email protected]>
Signed-off-by: Matei David <[email protected]>
Signed-off-by: Matei David <[email protected]>
Signed-off-by: Matei David <[email protected]>
Signed-off-by: Matei David <[email protected]>
Signed-off-by: Matei David <[email protected]>
Signed-off-by: Matei David <[email protected]>
Signed-off-by: Matei David <[email protected]>
Signed-off-by: Matei David <[email protected]>
Signed-off-by: Matei David <[email protected]>
Signed-off-by: Matei David <[email protected]>
Signed-off-by: Matei David <[email protected]>
Signed-off-by: Matei David <[email protected]>
Signed-off-by: Matei David <[email protected]>
Signed-off-by: Matei David <[email protected]>
Signed-off-by: Matei David <[email protected]>
Signed-off-by: Matei David <[email protected]>
Signed-off-by: Matei David <[email protected]>
…t-impl Signed-off-by: Matei David <[email protected]>
Signed-off-by: Matei David <[email protected]>
Signed-off-by: Matei David <[email protected]>
Signed-off-by: Matei David <[email protected]>
Signed-off-by: Matei David <[email protected]>
Signed-off-by: Matei David <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1449 +/- ##
=======================================
+ Coverage 74.3% 74.7% +0.5%
=======================================
Files 78 79 +1
Lines 6689 6868 +179
=======================================
+ Hits 4964 5125 +161
- Misses 1725 1743 +18
|
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.
This is huge! Amazing work. I've left some initial thoughts on the public interface and some of the internals.
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.
a couple more comments
Co-authored-by: Eirik A <[email protected]> Signed-off-by: Matei David <[email protected]>
Signed-off-by: Matei David <[email protected]>
Signed-off-by: Matei David <[email protected]>
Signed-off-by: Matei David <[email protected]>
Signed-off-by: Matei David <[email protected]>
Signed-off-by: Matei David <[email protected]>
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.
looks great. nothing major stands out so 👍 from me
some minor nits, feel free to fix/ignore
Signed-off-by: Matei David <[email protected]>
Signed-off-by: Matei David <[email protected]>
This was never properly usable without further integration, and as such was never stabilised. It is replaced via #1449 Signed-off-by: clux <[email protected]>
* Remove abandoned `StreamSubscribe` implementation This was never properly usable without further integration, and as such was never stabilised. It is replaced via #1449 Signed-off-by: clux <[email protected]> * forgot two imports Signed-off-by: clux <[email protected]> --------- Signed-off-by: clux <[email protected]>
Motivation
Stream sharing has been missing from the library for some time now. We've had a few attempts at making it happen, e.g. new controller interfaces (such as
Controller::for_stream
) and a stream subscriber adapter. #1189 describes the original issue we sought to solve before shipping shared streams: getting all of the interfaces to play well together.This change is a departure from the original issue described in #1189. After some exploration, it became a bit clearer that the stream subscriber interface may not be the best primitive to use:
This PR (and most of the exploration done as part of it) aims to address these two issues by changing and adding new interfaces.
Solution
Broadly speaking:
As a general rule of thumb, the solution here should be backwards compatible and avoid breaking current functionality. For now, it made more sense to introduce more duplication to allow for more flexibility in the future to change the current implementation (or in other words this should be marked as experimental).
More focused:
ObjectRef
) on the broadcast channel.ReflectHandle
contains a store reader and a broadcast receiver. It implementsStream
and yields K-typed objects. The stream implementation reads a reference from the broadcast channel and retrieves the object from the store.Contract:
new_with_dispatch
, otherwise the broadcast channel will never be used.reflect()
. More subscribers can be created after by cloning the initial handle.Controller::for_shared_stream
).Potential future work:
Controller::for_owned_shared
, naming TBD ofc).dispatch_event
withapply_watcher_event
Feedback welcome! Especially if we can simplify the current implementation. Some things may be out of place for now. I think in the spirit of keeping this as flexible as possible, my recommendation would be to duplicate code until we can get this in the hands of people that have more niche use cases or that can test it out at scale.
There's a big commit history for people curious on how this evolved over time. Ultimately settled on some approaches suggested in #1426.