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" <[EMAIL PROTECTED]> 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* <[EMAIL PROTECTED] <mailto:[EMAIL PROTECTED]>> >> 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" >
