[GitHub] [geode] nonbinaryprogrammer commented on pull request #6025: GEODE-8936: add INCR to list of supported commands

[GitBox <gi...@apache.org>]

nonbinaryprogrammer commented on pull request #6025:
URL: https://github.com/apache/geode/pull/6025#issuecomment-777691542


   > Something else to think about is that we changed `INCRBYFLOAT` to parse a BigDecimal rather than a long. I'm not sure if we want to change all INCR/DECR related commands to parse BigIntegers or BigDecimals? (If we did, then we wouldn't have to worry about the max/min 64 bit signed integer value tests... it would be different from Redis, but we are already different due to the change in `INCRBYFLOAT`)
   > 
   > **Update:** Jens told me BigDecimals were used in `INCRBYFLOAT` so that floating point would be more accurate... and keeping long for other INCR/DECR ops will be faster performance-wise. So unless we think there's a strong use case (maybe page view count?) or selling point for being able to INCR/DECR larger values than Redis, maybe that change doesn't make sense.
   
   I think that in this case we should favor performance over consistency with another command, especially since that command isn't consistent with native redis. thank you for looking into that though


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org