-
Notifications
You must be signed in to change notification settings - Fork 288
Add support for native async stream sources #357
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
Conversation
0d1013f
to
5a2c369
Compare
Thanks! Can you say a bit about |
/// Data and progress events of the native stream. | ||
pub enum Event<T, D> { | ||
/// Indicates that timestamps have advanced to time T | ||
Progress(T), | ||
/// Indicates that event D happened at time T | ||
Message(T, D), | ||
} |
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.
We should talk through this! There is already a similar enum in the capture
module, and it is a bit more rich than this. In particular, this enum isn't able to capture partially ordered times, where there may be a set of capabilities rather than just one.
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.
I did see that one but wasn't sure what the Vec is for. Let's talk about it tomorrow
It's still smaller than the full |
b1a39d0
to
e578886
Compare
Signed-off-by: Petros Angelatos <[email protected]>
Signed-off-by: Petros Angelatos <[email protected]>
Thanks! I'll get cracking on this promptly. The main ? for me is getting my head around |
b83b835
to
2370106
Compare
B: std::borrow::Borrow<T>, | ||
F: IntoIterator<Item=B>, |
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.
Minor, but can this be
F: IntoIterator,
F::Item: std::borrow::Borrow<T>,
or is the generic argument B
necessary / helpful?
Signed-off-by: Petros Angelatos <[email protected]>
2370106
to
6b6ef42
Compare
ok, I did some minor cleanups and fixed the example code. It should be good to merge I think! |
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.
The futures goop looks great to me. No opinion on the Event
protocol!
cap_set.downgrade(time); | ||
} | ||
Some(Event::Message(time, data)) => { | ||
output.session(&cap_set.delayed(&time)).give(data); |
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.
Minor, but this will eventually have throughput issues. At least, timely can send data around much faster than it can mint capabilities. It may eventually be smart to have the D
of the enum implement IntoIterator<Item = D2>
, though perhaps .. that is just a flatmap after this. Idk. Doesn't block anything here, but wanted to call out!
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 good to me. Various comments that are good for future discussion (lol not intended nvm sry).
…)" This reverts commit 935b7e9.
…)" This reverts commit 935b7e9.
Example usage: