Fixed return value from delete command. #190
Conversation
This would be much cleaner avoiding the flag and dropping |
|
I am open to the idea that "NOT_FOUND" is a failure, as no deletion took place. I'd go so far as to say I think the current behavior is wrong, and question the sanity of the maintainer back in 2010, but I imagine the thinking was along the lines that it's safer to return success than possibly have code break because it got a "0" and was thinking there was a memcache problem. Strictly from reading the docstring: "Deletes a key from the memcache" and "Returns nonzero on success", I think NOT_FOUND is not success. It also isn't a failure. The issue is that delete() also returns 0 when the server is down. So having NOT_FOUND return "0" means that you can't tell the difference between a negative (key is not in server) and an unknown (I wasn't able to check for the key in the server). But I'd be willing to call that an issue in the API contract ("return non-zero on success"). Without adding a new API endpoint with a different contract (say, returning deleted/not_found/failed), I think this may be our best bet: honoring the contract. I'm struggling to find a place where NOTFOUND returns 0 would be a breaking change. Opening for discussion. |
|
Yes. It's a little bit messy.
I think most people are probably not checking the return value of Also, the service going away typically will have wider issues than whether this particular command succeeded. It feels more useful to know where a key you expected to be present was successfully deleted or not there as you expected it to be. I'll update this PR to treat |
The `delete()` command currently returns `1` if the response from the server is either `'DELETED'` or `'NOT_FOUND'`. This differs from other implementations, e.g. `pymemcache` and `pylibmc`, and doesn't seem to make sense given that these are the only two documented return types from the protocol for the delete command. It seems that `delete()` was changed to explicitly consider both responses as successful way back in 2010, but that change only seemed to double down on the behavior that was being reported in the issue. See https://bugs.launchpad.net/python-memcached/+bug/471727. The concern was avoiding breaking backward compatibility. It was possible to work around this by calling `_deletetouch()` and passing in the `expected` responses, but that was broken by the change to remove `time` in ab668ed. Making the change to interpret `'NOT_FOUND'` as `0` doesn't cause any test failures, is consistent with the docstring which states that that the command returns non-zero on success, and also brings consistency with other implementations. Fixes #170.
|
I've now updated this as discussed. |
|
LGTM, thanks for working on this! |
The
delete()command currently returns1if the response from the server is either'DELETED'or'NOT_FOUND'. This differs from other implementations, e.g.pymemcacheandpylibmc, and doesn't seem to make sense given that these are the only two documented return types from the protocol for the delete command.It seems that
delete()was changed to explicitly consider both responses as successful way back in 2010, but that change only seemed to double down on the behavior that was being reported in the issue. See https://bugs.launchpad.net/python-memcached/+bug/471727. The concern was avoiding breaking backward compatibility.It was possible to work around this by calling
_deletetouch()and passing in theexpectedresponses, but that was broken by the change to removetimein ab668ed.To avoid issues with backward compatibility, I've added astrictargument which can be enabled to return a non-successful response fromdelete()for'NOT_FOUND'.Further, making the change to interpret
'NOT_FOUND'as0doesn't cause any test failures. I've added an assertion to check deletion of a non-existent keyusing both strict and non-strict.Fixes #170.