Add memcached #33
Add memcached #33
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #33 +/- ##
==========================================
+ Coverage 83.00% 83.51% +0.50%
==========================================
Files 10 10
Lines 1183 1468 +285
==========================================
+ Hits 982 1226 +244
- Misses 137 169 +32
- Partials 64 73 +9 ☔ View full report in Codecov by Sentry. |
leeym
force-pushed
the
add-memcached
branch
3 times, most recently
from
273c9b4
to
b2ed121
Compare
| case <-done: | ||
| if err != nil { | ||
| if errors.Is(err, memcache.ErrNotStored) { | ||
| return s.Increment(ctx, prev, curr, ttl) |
There was a problem hiding this comment.
It seems like we should return an error in this case and let the client decide how to handle it: otherwise we may end up in a loop
There was a problem hiding this comment.
The recursive Increment is trying to solve a race condition.
If two callers both Increment (line 195) at the same time while the counter doesn't exist, they both fail with ErrCacheMiss and Add (line 202) to initiate counter with value 1. Only one of them will succeed and the other will fail with ErrNotStored. The failing one can recursively call itself and do Increment (line 195) again, and the second time it should work as the counter is created already.
If you feel strongly against this implementation I can return error instead, otherwise I prefer to keep it as is.
There was a problem hiding this comment.
Could you please provide more details about this:
Only one of them will succeed and the other will fail with ErrNotStored.
How is it guaranteed that at most 1 client will be able to add the key to memcached?
| case <-done: | ||
| if err != nil { | ||
| if errors.Is(err, memcache.ErrNotStored) { | ||
| return s.Increment(ctx, prev, curr, ttl) |
There was a problem hiding this comment.
Could you please provide more details about this:
Only one of them will succeed and the other will fail with ErrNotStored.
How is it guaranteed that at most 1 client will be able to add the key to memcached?
This PR is to add support for memcached, including: