search

jaysonsantos / python-binary-memcached

A pure python module (thread safe) to access memcached via it's binary protocol with SASL auth support.
MIT License
165 stars 57 forks source link

Surface 'server_disconnected' state to API consumers #238

Open tonycosentini opened 3 years ago

tonycosentini commented 3 years ago

Would it be possible to surface the cases where there are connectivity errors?

For example with get right now, it returns None when there is a connectivity error and there is no way to distinguish if the key is actually missing or if there was a connectivity error.

I'd be happy to open a PR that adds this functionality via raising some kind of exceptions. I think to keep API compatibility enabling this would need to be an option when creating a client.

jaysonsantos commented 3 years ago

Hey there, I try to mimic what happens on pylibmc for example. Do you know what it returns? I guess that if you want this behavior, you could extend Protocol and on _connection_error you could raise the exception. Right now, when the socket is disconnected, disconnect is called so on the next get it should try and reconnect and as this is caching the data is dealt with in a way that it can be recomputed when cache fails, I think that it is better than raising an exception and breaking the client's app flow. If you want to distribute the load and increase availability, I would suggest using the DistributedClient because it will spread the load based on the key so if a caching server fails, it should affect only a portion of your load

tonycosentini commented 3 years ago

Hey @jaysonsantos, thanks for the guidance here!

I think pymc is raising ValueErrors - I'm not super familiar with the library, but looking at the C source, it looks like it calls PyErr_SetString in a bunch of cases.

For some additional context - we are using the distributed client. Our use case is using this library + flask-limiter. With flask-limiter, there are multiple get memcache calls that need to succeed (for example, get a rate limit expiration). If one of them errors, it's possible to end up extending rate limiting windows. This is rare, but can happen often depending on the request throughput you are doing. (Sorry if this is super hand-wavey).

Anyway, I'll try extending the protocol like you said. If it works out for our flask-limiter case I can open a PR to try to introduce the behavior without breaking the API (maybe make it opt-in or something).