-
Notifications
You must be signed in to change notification settings - Fork 65
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
Define Delete to be idempotent #104
Define Delete to be idempotent #104
Comments
For example, see: https://github.com/ipfs/go-datastore/pull/103/files#diff-42ea5667b327bb207485077410d5f499R42. I could fix that, but it would require a bunch of extra state I don't really want to track (and it would cause a failed delete to fail the entire batch. |
I don't have a strong objection. I would prefer a way to communicate back that a key is not found if that information is known, but as this may complicate things, this is not a hard requirement. |
The user can still call |
Generally, I agree. DELETEs are idempotent in SQL, leveldb, badger, redis, but some databases will report the number of keys deleted. Maybe we should add capability for returning an For the record,
In most of cases, yes. But ultimately this depends on the application logic. For example, in the Datastore TTL shim I've half-baked, we have two keys per TTL entry (for good reasons), and we might want deletes pertaining to a single TTL entry to be as atomic as possible. |
Ah, sorry, I meant we don't want to abort if the delete failed because the item wasn't there. We probably do want to abort on other errors.
So we'd return:
(where we'd only return an error if something went wrong) Personally, I'm not sure if implementing that is worth the work/complexity. Making DELETE idempotent is quite easy:
Basically, we can do this slowly without having to fix everything up-front. Changing the method signature, on the other hand, will take a bit more work. We could still do it, I just want to make sure the change is well motivated (i.e., we have a use-case where we actually care about this). If we do run into a case where we'd like to know, we can change the signature later (well, it'll be a breaking change but it'll be a breaking change either way). |
@Stebalien I agree this is over-complicating things. I think the best to not worry about for now. A better way to handle this is to query the datastore afterwards for the status of the last operataion, but this will require the use of handles, which is something we may want to do anyway down the road. |
i like @raulk's suggestion about returning the number of keys deleted... DBs that support versioning (badger) could potentially delete more than one key. so i'd think of it as <0, 0, >0 |
@raulk @kevina @bigs so, are you fine making delete idempotent (for now) where, sometime in the future, we could consider adding a delete count? Really, I'm just trying to (a) simplify some code that tries to hard to track whether or not the operation succeeded and (b) trying to nail down semantics (I hate undefined behavior). |
@Stebalien that fine with me. |
absolutely. fine with me!
…On Oct 24, 2018, 3:57 PM -0400, Kevin Atkinson ***@***.***>, wrote:
@Stebalien that fine with me.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
Idem. I have no immediate use case right now for the delete count. |
SGTM, in my book |
I was bitten by this one. In particular, the |
Another one not idempotent is flatfs. |
All done! |
Moving from: ipfs/go-ds-flatfs#30
This boils down to:
Proposal: Define it to be idempotent (i.e., return
nil
when the key isn't found).The text was updated successfully, but these errors were encountered: