Skip to content

Provide a mechanism for setting the version Option #1236

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Provide a mechanism for setting the version Option #1236

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions kernel/src/snapshot/builder.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,12 @@ impl SnapshotBuilder {
self
}

/// Set the `version` [Option] explicitly.
pub fn set_version(mut self, version: Option<Version>) -> Self {
self.version = version;
self
}
Comment on lines +46 to +50
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about at_version_opt (seems to match existing API better)? Or perhaps just unifying it all with at_version to take impl Into<Option> like @roeap says?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we had a decent bit of discussion on naming already - i'll let others chime in cc @nicklan @scovich @roeap

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO the real problem is letting people set the version at all, as if that were somehow orthogonal to all the other information that could be passed (e.g. set version to 10 but pass P&M for latest version 15). Which points to having a builder with two different constructors -- one for a latest-version builder and one for an as-of-version builder. But that doesn't actually prevent the caller from passing latest P&M to a time travel builder, so meh.

Meanwhile, I'm totally fine with impl Into<Option>, we've used that trick elsewhere -- notably when setting start and end versions for CDF -- for the exact same papercut-avoidance that @rtyler aims for here.

We can adjust the API further if/when we decide to fix the larger ergonomics/footgun issue (which first requires having some idea of how to fix it, still TBD).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yea and to add the current proposal for the "different kind of versions": we will have a builder which bifurcates into two different types based on if you do an at_version(version) or with_metadata(P, M, version). see #1189 (comment)


/// Create a new [`Snapshot`] instance.
///
/// # Parameters
Expand Down
Loading