storehaus-memcache: pass ttl for MergeableMemcacheStore CAS calls #262
Conversation
|
@franklinhu we'll want to make the same adjustment to the call to |
franklinhu
force-pushed
the
franklin-memcache-fix
branch
from
2b6cd04 to
fafa6d8
Compare
|
@danielhfrank Thanks, fixed in latest rev |
|
Thanks for the fix! Will merge when green. |
| @@ -77,7 +77,7 @@ class MergeableMemcacheStore[K, V](underlying: MemcacheStore, maxRetries: Int)(k | |||
| inj.invert(cbValue) match { | |||
| case Success(v) => // attempt cas | |||
| val resV = semigroup.plus(v, kv._2) | |||
| underlying.client.cas(key, inj.apply(resV), casunique).flatMap { success => | |||
| underlying.client.cas(key, 0, underlying.ttl.fromNow, inj.apply(resV), casunique).flatMap { success => | |||
There was a problem hiding this comment.
Actually, the second arg should be underlying.flag instead of 0?
There was a problem hiding this comment.
I have no idea what flag does, but this would change the existing behavior right? Should we also pass flags into CAS?
There was a problem hiding this comment.
Found an explanation here
https://github.com/memcached/memcached/blob/master/doc/protocol.txt#L155
and here
http://stackoverflow.com/questions/12228097/what-is-the-flags-arg-in-finagle-memcached-set-method
The flag is opaque to the server and is returned back along with the value when reading. For correctness, it looks like we should set the same flag as the underlying store does for set calls. It's probably been working fine all this while because of the default being 0 in MemcacheStore. Not your change, but would be nice to fix this as well. What do you think?
There was a problem hiding this comment.
Ah, didn't notice your latest rev. Should be added to the cas call as well I think.
|
Fixed the review feedback. Looks like the test failures are unrelated to the memcache changes |
|
restarted the build. |
|
Green now. Merging, thanks. |
storehaus-memcache: pass ttl for MergeableMemcacheStore CAS calls
MergeableMemcacheStore was not passing the right TTL setting, and thus was using the default of Time.epoch which never expires the item.
cc @danielhfrank