[PATCH] "incr" supports wraparound

[Marc]

I think this patch is right.  docs/protocol.txt clearly states it's meant to
be unsigned.  I think wrapping to zero best satisfies the principle of least
astonishment.

FWIW, I think the strtol instead of strtoul is my fault.  I'm not sure how I
missed that.  In my defense, I'm guessing I just tried to preserve the
signedness of the orignal implementation.

On 8/16/07 7:39 PM, "Evan Miller" <emiller at imvu.com> wrote:

> Clint Webb wrote:
>> Why would you *want* a wraparound when incrementing?
>> Quite the opposite for me, I do not want it to wrap around, because then
>> I would require additional code to check that.  Right now, very
>> simply... if my increment fails, I reset all the data that is associated
>> with that key, and start again.
> 
> Wraparound might actually be a better solution for you, because it
> solves the concurrency problems. Currently, several clients that
> simultaneously attempt to increment past the size limit will all receive
> errors, and they will all attempt to run your reset logic. I don't know
> about your specific use-case, but for us this means that increments get
> lost in the meantime (multiple clients reset the counter to 1).
> 
> With wraparound, only one client will receive a "0" return value, and so
> only one client will run the additional reset logic.
> 
> It's true that clients which rely on the increment errors will need
> tweaking, but overall wraparound makes Memcached more useful and robust
> as a generic counter service.
> 
> It would be even more useful if calling "incr" on a non-existent key set
> the value to "1" in order to avoid race conditions similar to what I
> describe above, but that's a different can of worms...
> 
> Evan
> 
>> 
>> Although, it is possible that treat a '0' returned as a wrap failure in
>> your scenario, it is easier for me to treat the wrap error the same as
>> if the key had expired, or the service had been restarted, causing me to
>> replenish the data that appears to be missing.
>> 
>> On 8/17/07, *Evan Miller* <emiller at imvu.com <mailto:emiller at imvu.com>>
>> wrote:
>> 
>>     Currently Memcached gives an error when incrementing large counters:
>> 
>>     set foobar 0 0 10
>>     2147483647
>>     STORED
>>     incr foobar 1
>>     2147483648
>>     incr foobar 1
>>     CLIENT_ERROR cannot increment or decrement non-numeric value
>> 
>>     I believe this is a bug. Attached is a patch to make counters wrap
>>     around the 2**32 mark, similar to counters in routers. New behavior:
>> 
>>     set foobar 0 0 10
>>     4294967294
>>     STORED
>>     incr foobar 1
>>     4294967295
>>     incr foobar 1
>>     0
>> 
>>     A CLIENT_ERROR will continue to be returned if a value above 2**32 is
>>     incremented, but the "incr" command will never push the value over that
>>     mark.
>> 
>>     A future version of Memcached should probably use a 64-bit counter
>>     instead, but the 32-bit limit is in line with the existing docs. ("The
>>     data for the item is treated as decimal representation of a 32-bit
>>     unsigned integer.")
>> 
>>     The patch also adds "const" keywords to arguments of do_add_delta and
>>     mt_add_delta, to be consistent with the header file.
>> 
>>     Documentation and tests have been updated.
>> 
>>     Evan Miller
>>     IMVU, Inc.
>> 
>> 
>> 
>> 
>> -- 
>> "Be excellent to each other"
>