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

Make createCheckpointAndClose part of public interface #142

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

adamgundry
Copy link
Contributor

Fixes #112. This exposes createCheckpointAndClose via Data.Acid[.Abstract] and makes it work for all backends:

  • On the Local backend it works as it always did, creating a checkpoint and not writing any events afterwards.
  • On the Memory/Remote backends it simply calls the create checkpoint operation followed by the close operation. (Previously calling createCheckpointAndClose on these backends would throw an error as the downcast failed.)

I've also removed the dynamically-typed stuff in a separate commit, since it is not used for anything else in acid-state. It is conceivable that it might be used by dependent packages, though, so we could imagine keeping this in. I'm not convinced it provides a good interface though.

This needs a changelog update, assuming there is consensus that this is a sensible change.

@adamgundry
Copy link
Contributor Author

I suspect the CI failure on AppVeyor to be spurious; it looks like some kind of timeout setting up GHC 8.2.

@stepcut
Copy link
Member

stepcut commented Apr 7, 2022

It looks like the subType stuff was added in this commit,

bcc6c86

But no explanation anywhere of why or what the intended use case is. If I had to guess, the idea was to allow various backends to store other information in the AcidState handle that is specific to that backend?

@adamgundry
Copy link
Contributor Author

f I had to guess, the idea was to allow various backends to store other information in the AcidState handle that is specific to that backend?

Yes, I think that's the idea. Certainly that is how the Local backend uses it prior to this PR. It seems acid-state-dist also uses it similarly, so this would be a breaking change at least for acid-state-dist (though that doesn't look like it has been kept up to date with acid-state anyway).

I dislike this design as a mechanism for extension, because it means there is no static type distinction between AcidStates with and without the extension. For example, prior to this PR it is possible to call createCheckpointAndClose on a Memory state and get a runtime exception. I think it would be cleaner for extensions like this to define new types, then their own versions of functions like query/update (that work on the new type) and/or a conversion back to an AcidState.

But the question remains as to whether we should keep this for backwards compatibility anyway.

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.

Add createCheckpointAndClose to AcidState interface?
2 participants