dice icon indicating copy to clipboard operation
dice copied to clipboard

Report inconsistency in the command `LRU`

Open arpitbbhayani opened this issue 1 year ago • 14 comments

This issue is all about ensuring we are as close to Redis as possible. The command in focus for this issue is LRU.

Go through the official documentation of the command LRU on Redis and identify the inconsistencies. The inconsistencies could be in

  1. unhandled edge case
  2. unexpected behavior
  3. unsupported option

Because we are trying to be compatible with Redis v7.2.5, I would recommend you try out different variants of the command with different inputs on that specific version. The instructions on running Redis v7.2.5 locally

Once you find the discrepancy, you can either

  1. raise an issue on Dice repository with details, or
  2. try to fix it yourself and raise a PR

If you are raising the issue, make sure you provide the details such as

  1. use the template and provide the following details
  2. steps to reproduce (series of commands)
  3. observed output on DiceDB
  4. observed output on Redis v7.2.5

Also, feel free to update the documentation and raise the PR in the docs repository.

You will need to go deeper into the command make sure you are covering all cases and reporting the inconsistencies or fixing them. The deeper the work, the better our stability will be. Also, it is possible that we do not find any discrepancies, so please mention the same in the comment on this issue. Mention the PR or issue links that you create under this issue.

arpitbbhayani avatar Aug 21 '24 17:08 arpitbbhayani

Hi @arpitbbhayani can i take this up? thanks!

Abh-ay avatar Aug 22 '24 10:08 Abh-ay

@Abh-ay go for it!

arpitbbhayani avatar Aug 22 '24 14:08 arpitbbhayani

Hi @arpitbbhayani @JyotinderSingh @soumya-codes , As I saw in redis docs and in cli I'm not able to find as such regarding this LRU command. While they implemented a various policy and evict key according to the same. Refer : https://redis.io/blog/cache-eviction-strategies/. I'm bit confused why we implemented LRU command? :thinking: as of now should we go for implement logic of various policies and then based on these policies we will implement this LRU logic? And as currently on latest master LRU command deletes all key from db image

Abh-ay avatar Aug 26 '24 13:08 Abh-ay

Screenshot 2024-09-03 at 3 43 50 AM

@Abh-ay I guess you're referring to these eviction policies from Redis. I would suggest to wait for till we refurbish the store and maybe then pick one-by-one eviction policies. @soumya-codes your thoughts?

AshwinKul28 avatar Sep 02 '24 22:09 AshwinKul28

Yes @AshwinKul28 I referred this doc, Just want to confirm are we having any plan to raise separate issues for different policies or implement one by one in this issue only? and also this issue for report inconsistency in LRU only but I don't think we need to verify this yet. and any plan to prioritize some policies to implement or any index which we need to follow for implement mentioned policies? let me know so we can proceed further...

Abh-ay avatar Sep 05 '24 16:09 Abh-ay

Hi @arpitbbhayani , I would like to work on this, can you assign me this one.

Nijin-P-S avatar Sep 13 '24 13:09 Nijin-P-S

hi @arpitbbhayani I would like to work on this, can you please assign this to me? It's the only remaining unassigned issue which is a good-first-issue

srikomm avatar Sep 19 '24 20:09 srikomm

Hey @arpitbbhayani , can I take this up?

sathwikreddygv avatar Oct 06 '24 12:10 sathwikreddygv

@arpitbbhayani I'm still waiting to get your response, want to take this up

srikomm avatar Oct 07 '24 11:10 srikomm

@srikomm @arpitbbhayani @JyotinderSingh can I please take this up?

shashank-priyadarshi avatar Oct 23 '24 11:10 shashank-priyadarshi

@shashank-priyadarshi I'm waiting from a long time to take this up :)

srikomm avatar Oct 23 '24 12:10 srikomm

@shashank-priyadarshi Can i take this up?

berabhishek avatar Nov 10 '24 15:11 berabhishek

Hi @arpitbbhayani - Can I pick if no one is working on this?

Tanmay1201 avatar Nov 13 '24 16:11 Tanmay1201

Hi @arpitbbhayani ,

I’ve reviewed the Redis LRU command and tested its behavior on Redis v7.2.5. I’ve identified potential discrepancies and have ideas to improve compatibility with DiceDB. Please assign this issue to me, and I’ll ensure all edge cases are thoroughly addressed.

piyushhhxyz avatar Nov 24 '24 14:11 piyushhhxyz