Skip to content

Conversation

frankmcsherry
Copy link
Member

@frankmcsherry frankmcsherry commented Sep 18, 2025

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 a Session 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 an Option<T> in some longer-lived state. We could reintroduce it though, by adding the Option to the OutputBuffer.

@frankmcsherry
Copy link
Member Author

frankmcsherry commented Sep 19, 2025

There's a thing I'd like to fix, which is the ability to group InputHandle::next output into something that iterates through (time, iter) where iter is some iterator over containers. This would give a reasonable answer to the session chaining that we lost with this PR, but would actually be a "better" way to do it. Pretty small change, but interested to talk through this before doing too much about it. It should be able to do done on main rather than in the PR, for example.

Edit: did this.

Copy link
Member

@antiguru antiguru left a 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.

@frankmcsherry
Copy link
Member Author

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

@frankmcsherry frankmcsherry marked this pull request as ready for review September 21, 2025 12:18
@frankmcsherry
Copy link
Member Author

Some light post-review changes: rolled back the &mut OutputBuilderSession -> OutputBuilderSession change, at it seems to only introduce a bunch of mut output annotations and is noise at the moment. It does seem to eventually make sense, but I don't see that it changes the software engineering at all (it "makes sense" because the type is already a mutable borrow, and we are passing ownership of it in to the closure and dropping it when it returns).

Also, partition.rs was pivoted over to use the new input/output combination. I think it looks a bit tighter, but I think there are further conversations we could have about whether staging records in a vec or a container builder makes more sense (I think it comes down to "do we want to have a container builder per-output" where the current code is "no" but it could be made in to "yes" without much work).

@frankmcsherry
Copy link
Member Author

Ok, merging with the potential that I need to learn about interactive rebases in order to remove. :D

@frankmcsherry frankmcsherry merged commit c676389 into TimelyDataflow:master Sep 21, 2025
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants