[GitHub] [incubator-kvrocks] PragmaTwice commented on pull request #1489: Add Redis-compatible cursors for `SCAN` commands

["PragmaTwice (via GitHub)" <gi...@apache.org>]

PragmaTwice commented on PR #1489:
URL: https://github.com/apache/incubator-kvrocks/pull/1489#issuecomment-1585533419

   Thanks for your contribution!
   
   Several points after a short glance:
   - seems the ring buffer is shared between all connections, so just be curious that, will the functionality of `SCAN` get break if we have lots of client connections simultaneously? And could we make it duplicate per connections or able to adjust the buffer size dynamically?
   - if some keys are deleted while the `SCAN` operation is executed, will the key properties of `SCAN` be preserved? (refer to "Scan guarantees" in https://redis.io/commands/scan/#scan%20guarantees)
   - self-increase cursor may cause informantion leaks: one client can retrieve some informantion of the current executed commands from another client (partially same as the first point)
   - seems the mutex can be replaced by rwlock or atomic variables
   
   cc @mapleFU @torwig 


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

To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org

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