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

Handling 404s for DELETEs #7

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

Handling 404s for DELETEs #7

wants to merge 3 commits into from

Conversation

nnuss
Copy link

@nnuss nnuss commented Apr 13, 2015

It seems sane to me to accept HTTP 404 as success for delete()s as the result is what we want.

I toyed with returning "0E0" instead but decided it probably benefitted few people.

Cheers,
-Nate

nnuss added 3 commits April 13, 2015 17:27
As currently defined ShardedKV::Storage::Rest suceeds on: 2xx and fails
on any other HTTP response code.
@xant
Copy link
Owner

xant commented Apr 14, 2015

one would argue that 404 is 'NOT FOUND' so no 'delete' actually took place ... are you sure this is the desired behaviour?

@nnuss
Copy link
Author

nnuss commented Apr 15, 2015

I did consider that ; I was weighing returning 0E0 in similar fashion to DBI. But ReplicatedRest reduces the result down to a 0/1 anyway based on max_failures. I'm still open to that idea if you think direct SKV::S::Rest users would benefit from being able to detect this case vs. surprising them with handling the "true" zero pattern.

My thinking is that when calling delete() I care foremost about the behavior:
I expect to have the key/object removed/absent upon success - or - receive an error and have an expectation that the key may [read: probably does] persist.

Since delete is idempotent (multiple deletes achieve the same state a single delete) I think the client(s) benefit from not having to defend against 404-as-failure. Particularly since this type of "failure" is not easily distinguishable to the caller doing delete().

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.

2 participants