-
Notifications
You must be signed in to change notification settings - Fork 288
Pivot container builders above capability checking #715
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
Pivot container builders above capability checking #715
Conversation
8c858c4
to
02e18e1
Compare
02e18e1
to
f855749
Compare
There's a thing I'd like to fix, which is the ability to group Edit: did this. |
8482b64
to
9b5d5b9
Compare
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'm good with merging this, and prefer addressing the downstream changes in one go rather than piece-by-piece.
I'm also up for waiting a bit if you prefer, to take stock of the async modifications that may be needed. Happy to help with the downstream adoption, of course. Also not interested in doing that more than once. :D |
c27a14b
to
9b5d5b9
Compare
Some light post-review changes: rolled back the Also, |
Ok, merging with the potential that I need to learn about interactive rebases in order to remove. :D |
Timely has historically had a built in
pushers::Buffer
which houses a container builder, and is essentially mandatory. This is confusing for operators that do not require container builders, and from an abstraction point of view, the choice of how to build containers seems at a higher level than e.g. capability verification.This PR pivots these around, introducing a
pushers::Progress
that validates capabilities, around which we wrap container building abstractions.One regression that we should consider testing is that
.session(&cap)
returns aSession
which will flush the container builder when dropped, which is unlike our current behavior where .. it doesn't flush anything and if you create a new session with the same time it just picks up where it left off. I think that is a glitch, and creates weird logical states where you have anOption<T>
in some longer-lived state. We could reintroduce it though, by adding theOption
to theOutputBuffer
.