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

Add createCheckpointAndClose to AcidState interface? #112

Open
adamgundry opened this issue Nov 13, 2018 · 2 comments · May be fixed by #142
Open

Add createCheckpointAndClose to AcidState interface? #112

adamgundry opened this issue Nov 13, 2018 · 2 comments · May be fixed by #142

Comments

@adamgundry
Copy link
Contributor

At the moment Data.Acid.Local has

createCheckpointAndClose :: (SafeCopy st, Typeable st) => AcidState st -> IO ()

but this type is a lie and the function will throw an error if called with a non-local state. Is there any reason why createCheckpointAndClose shouldn't be part of the abstract AcidState interface? This would make it easier to write backend-generic testing code.

It would also avoid the need for downcast/acidSubState etc.. Should we leave them in for backwards compatibility, as we don't currently offer another way to create a LocalState/RemoteState directly? Or should we offer alternative functions that construct the underlying sub-states in a type-safe way, and remove the dynamically-typed stuff altogether?

@stepcut
Copy link
Member

stepcut commented Feb 2, 2019

I know createCheckpointAndClose was added as an after thought. That might explain why it is not part of the interface. Or maybe there was some reason why it was only appropriate for the local backend. But if there was a reason, I don't know what it was.

@adamgundry
Copy link
Contributor Author

There is a question as to what createCheckpointAndClose should do in the remote case. I don't see any reason in principle that the remote interface couldn't support it, but that would require extending the protocol.

I've not done much with the remote interface and it is rather untested, so I'm not terribly keen to put a lot of effort into it. Thus I'd rather make the remote implementation do a non-atomic checkpoint before closing the state, and document that createCheckpointAndClose is backend-dependent.

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 a pull request may close this issue.

2 participants