[PATCH] "incr" supports wraparound

[Evan Miller]

Marc wrote:
> 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.

Hey no worries. At least there's a test now. :)

If wrapping around is going to break some application code out there, 
should we go ahead and up the counter size to 64 bits and make 
wraparound virtually a non-issue? That is, does anyone whose counters 
reach the billions rely on the counters' 32-bittedness?

Evan

> 
> 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"
> 
> 
>