Skip to content
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

feat: add fakeDurable option to assist durability conversion #5481

Merged
merged 1 commit into from
Jun 2, 2022

Conversation

FUDCo
Copy link
Contributor

@FUDCo FUDCo commented May 31, 2022

This change adds a fakeDurable option to the makeScalaraBigFooStore and defineDurableKind/defineDurableKindMulti calls. Collections or virtual objects that result from using this option are not actually durable but will appear durable for purposes of being inserted into another durable collection or durable object. Note that such objects will not survive upgrade -- they will be discarded on upgrade like any other non-durable object -- but can be used in code that is in the midst of conversion from using purely virtual objects to durable objects. By relaxing the requirement for transitive durability, we hope to ease the conversion effort by enabling objects that will eventually be durable to be used as if they were durable even though they're not durable yet.

Closes #5454

@FUDCo FUDCo added enhancement New feature or request SwingSet package: SwingSet tooling repo-wide infrastructure labels May 31, 2022
@FUDCo FUDCo requested review from warner and erights May 31, 2022 21:41
@FUDCo FUDCo self-assigned this May 31, 2022
Copy link
Member

@erights erights left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!!!!

Comment on lines +936 to +935
assert(
!durable || !fakeDurable,
'durable and fakeDurable are mutually exclusive',
);
Copy link
Member

Choose a reason for hiding this comment

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

Amount of duplicated code here is unfortunate. If there's a way to consolidate without making other things noticeably worse, it's probably worth it.

}
if (fakeDurable) {
assert(
durable,
Copy link
Member

Choose a reason for hiding this comment

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

A bit surprising since collectionManager uses flags of exactly these two names but insists that those are mutually exclusive. I don't have a suggestion, so it you don't think of an improvement, you can leave this as is.

Copy link
Member

@warner warner left a comment

Choose a reason for hiding this comment

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

This does what it says on the tin, so I'm ok with it landing.

But I'm not excited about this surviving in our codebase for very long: I'd like the companion ticket (about making it disabled by default) to consider removing it entirely once we've completed the durabilization transition. I'm hoping that future authors will be able to avoid the incremental conversion process.

@FUDCo FUDCo added the automerge:rebase Automatically rebase updates, then merge label Jun 2, 2022
@mergify mergify bot merged commit 43488e4 into master Jun 2, 2022
@mergify mergify bot deleted the 5454-fake-durable branch June 2, 2022 18:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge:rebase Automatically rebase updates, then merge enhancement New feature or request SwingSet package: SwingSet tooling repo-wide infrastructure
Projects
None yet
Development

Successfully merging this pull request may close these issues.

need fakeDurable option
3 participants