mbr / simplekv

A simple key-value store for binary data.
http://simplekv.readthedocs.io
MIT License
152 stars 50 forks source link

Copy API #40

Closed criemen closed 7 years ago

criemen commented 7 years ago

This PR adds store.copy() and store.rename() to the simplekv API. Backend support for these operations is not needed, as they can be simulated by get/put/delete.

Offering this through simplekv has the advantage for larger values to leverage specific API to deal with copies/renames more efficiently. For example in the S3 case, copying now does not need to download and upload the whole blob, which saves time, bandwith and (potentially a lot of) memory.

This depends on PR #39 for green tests.

mbr commented 7 years ago

This PR adds store.copy() and store.rename() to the simplekv API. Backend support for these operations is not needed, as they can be simulated by get/put/delete.

I disagree here - atomicity is a big problem in that case. While I am cautious to give guarantees, trying to limit the API to atomic operations is fairly important.

Offering this through simplekv has the advantage for larger values to leverage specific API to deal with copies/renames more efficiently. For example in the S3 case, copying now does not need to download and upload the whole blob, which saves time, bandwith and (potentially a lot of) memory.

I agree, that sounds like a worthwhile feature to have.

It seems like this would be a good fit for a mixin instead, adding this feature for backends that can support it. If one needs to emulate it, it would still be possible just add a wrapper?

criemen commented 7 years ago

Hi, I can understand that you care about atomicity of the operations and that a chain of operations is a problem here.

While I can refactor the two API calls into mixins, I do not see how I as lib user who does not need atomicity then can transparently get backends with copy/rename support - either using the backend API or, if not available, using the fallback. I'd rather avoid having to litter my code with checks ala "if isinstance(store, CopyStoreMixin): store.copy() else: fallbackcode". As I'm not well-versed in python, I might be simply missing a language feature allowing this use-case, but else I'd rather add a warning to the documentation of copy/rename which explicitly mentions atomicity as a concern.

mbr commented 7 years ago

While I can refactor the two API calls into mixins, I do not see how I as lib user who does not need atomicity then can transparently get backends with copy/rename support - either using the backend API or, if not available, using the fallback.

It would not be two mixins, rather a wrapper that adds the missing copy/rename features. Alternatively, if you want to stick with mixins completely, you can simply derive from the mixin and a suitable backend.

criemen commented 7 years ago

So do you prefer the solution now with the CopyRenameDecorator? There you have to explicitly enable the public API for a store, which hopefully leads to people reading the documentation about the non-atomicity of copy/rename.

criemen commented 7 years ago

TODO: Write documentation for the new mixin

criemen commented 7 years ago

After a discussion with @fmarczin we concluded that we build this back to just exposing an atomic copy operation if it is available.