Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Audit and make documentation for command LRU consistent #815

Open
arpitbbhayani opened this issue Sep 29, 2024 · 11 comments
Open

Audit and make documentation for command LRU consistent #815

arpitbbhayani opened this issue Sep 29, 2024 · 11 comments
Assignees
Labels
documentation Improvements or additions to documentation good first issue Good for newcomers

Comments

@arpitbbhayani
Copy link
Contributor

The documentation of the command [LRU](https://dicedb.io/commands/lru/) present in the docs could have become stale. We need your help to ensure that the documentation is complete, correct, and consistent.  Completing this issue will give you much-needed exposure to how the command is implemented and the different parameters it accepts. Hence, this is a great way to build a good understanding of the project and its functionality. While still making a significant contribution. Here's what you need to do:

  1. Go through the documentation of the command and run all the examples, making sure they all work as expected
  2. If the same command is present in Redis, then the expected output of the DiceDB command is the same as the Redis output
  3. If the command is not in Redis, then as per your judgment, raise an issue with this template or fix the documentation and raise a pull request.
  4. The documentation should contain the following sections in the following order
  • short and concise introduction paragraph about the command covering what it does
  • Syntax
  • Parameters: List all the parameters the command accepts, and ignore the section if there are no parameters
  • Return values: List all possible return values and under what condition
  • Behaviour: Describe the behavior of the command, what it does, how it does it, some internal specifics if any
  • Errors: List all possible errors the command can throw and under what condition
  • Examples: List all possible examples of the command and the expected output. Assume CLI implementation

Note: The title of the sections should be the exact strings mentioned above, like "Examples", "Return values", etc.
Note: The description of the command in Frontmatter of the command.md file is the first paragraph (introduction) of the command.

Go through the DiceDB/dice repository and explore the command implementation

  • to understand all possible return values
  • to understand all possible error values and conditions
  • to understand different parameters the command accepts

Please use the documentation of the SET command as the reference point and structure the
documentation of this command in a very similar way, including the following points

  • if you see any "Conclusion" section, remove it
  • the headers (h1, h2, h3) should be appropriately used
  • the CLI prompt should be 127.0.0.1:7379> and not anything else
  • Use markdown tables for parameters and return values (ref SET command documentation)
  • wrap any command or paramter in backticks (`) to highlight them
  • the section header should be capitalize as used in the SET command documentation

If you find any inconsistencies, please fix the documentation and raise the PR.
The core idea of this exercise is to ensure that the documentation is consistent, correct, and complete.

Make sure you comment on the other issues you created (if any) as a comment on this issue and also any PR (if any) that you created.
Thank you for picking this up and contributing to the DiceDB. It means a ton.

@arpitbbhayani arpitbbhayani added documentation Improvements or additions to documentation good first issue Good for newcomers labels Sep 29, 2024
@Srajan007
Copy link

@arpitbbhayani Can I take this up ?

@arpitbbhayani
Copy link
Contributor Author

@Srajan007 go for it. assigned. thanks for taking this up.

@Srajan007
Copy link

@arpitbbhayani Fixed it under this PR, the command LRU didn't exist in Redis.

@arpitbbhayani
Copy link
Contributor Author

Hello @Srajan007,

There has been no activity on this issue for the past 5 days.
It would be awesome if you keep posting updates to this issue so that we know you are actively working on it.

We are really eager to close this issue at the earliest, hence if we continue to see the inactivity, we will have to reassign the issue to someone else. We are doing this to ensure that the project maintains its momentum and others are not blocked on this work.

Just drop a comment with the current status of the work or share any issues you are facing. We can always chip in to help you out.

Thanks again.

@Srajan007 Srajan007 removed their assignment Nov 2, 2024
@paulwalrath
Copy link
Contributor

Unclear to me whether work is still needed on this issue. If so, I'd be willing to work on it.

@RhythmAgg
Copy link

@JyotinderSingh @arpitbbhayani May it take this up? I have also raised an issue regarding the same: #1263

@JyotinderSingh
Copy link
Collaborator

@JyotinderSingh @arpitbbhayani May it take this up? I have also raised an issue regarding the same: #1263

Assigned

@RhythmAgg
Copy link

RhythmAgg commented Nov 10, 2024

@JyotinderSingh The current documentation of the LRU command and the actual working differs.

  • The documentation assumes the command returns the list of keys which are candidates of eviction.
  • The actual implementation evicts all the LRU keys and returns clientio.RespOK ( 'OK' ) in all cases.

So I should make changes to the documentation following the current working of the command?

@vanshavenger
Copy link
Contributor

ig it should return the values as Ok won;t be beneficial

@tejasa97
Copy link

agreed. can i pick up the changes for this?
unless @RhythmAgg is still on track with this task

@RhythmAgg
Copy link

ig it should return the values as Ok won;t be beneficial

So should the LRU command evict the LRU keys and return the evicted keys to the client? In the documentation It doesnt say if the keys are evicted as well or only the information of the LRU keys is returned.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

7 participants