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

Add cron for cleanup of keys from DiceDB #32

Closed
lucifercr07 opened this issue Oct 5, 2024 · 26 comments
Closed

Add cron for cleanup of keys from DiceDB #32

lucifercr07 opened this issue Oct 5, 2024 · 26 comments
Assignees
Labels
enhancement New feature or request good first issue Good for newcomers stack -- go

Comments

@lucifercr07
Copy link
Contributor

lucifercr07 commented Oct 5, 2024

Description

Currently, there is no automated cleanup process for stale or unused keys stored in DiceDB within the playground-mono repository. This can lead to unnecessary memory consumption and inefficient resource usage over time, especially since we're having a global keypool currently.

We need to add a cron job that will periodically clean up all the keys in DiceDB using the SDK. The frequency of the cron job should be configurable, and by default, it should run every 15 minutes. This cron job should interact with the DiceDB using its SDK to delete all keys once configured time is reached.

Tasks

  1. Define Configuration for Cron Frequency
    • Add a configuration option in playground-mono for setting the cron frequency.
    • By default, the cron should run every 15 minutes (can be overridden by the config).
  2. Implement Cron Job
    • We can use time.Ticker with a Goroutine to implement cron
  3. Use the DiceDB SDK to interact with the database and cleanup keys.
  4. Handle Logging & Errors
    • Implement appropriate logging for the cron job to track the number of keys deleted, the success/failure of each job run, and any errors encountered during execution.
    • Ensure error handling doesn’t cause cron crashes and retries gracefully on failure.
  5. Create unit tests
  6. Create integration tests
    1. This should spin up a DiceDB server and interact with it.
    2. Please consider that for the integration tests we should ideally have a dice server spin up via Docker container in Go and then write integration tests. We can use Go Libraries like testcontainers-go for this.

Please feel free to comment on issue for more details.

@lucifercr07 lucifercr07 added good first issue Good for newcomers stack -- go enhancement New feature or request labels Oct 5, 2024
@RishabhC-137
Copy link
Contributor

I will take this @lucifercr07

@tarun-kavipurapu
Copy link
Contributor

Can you assign this to me @lucifercr07

@lucifercr07
Copy link
Contributor Author

lucifercr07 commented Oct 5, 2024

@RishabhC-137 @tarun-kavipurapu thanks a lot for your interests. I believe both of you have issues assigned already.
Let's give chance to others from the community to contribute.
Feel free to comment once other issues are done, would happy to assign if this issue is still unassigned.

@RishabhC-137
Copy link
Contributor

Hi @lucifercr07 , thanks for considering my interest! I haven't started working on the previous issue assigned to me, so I’m happy to unassign myself from it if possible. Could you please assign this issue to me instead?

@Shimanshu83
Copy link
Contributor

Hi @lucifercr07 can you assign this issue to me. The assigned previous issues are fixed and I have written comment on it.

@Shimanshu83
Copy link
Contributor

One more thingh I write lot of cron job in my workplace too in golang.

@lucifercr07
Copy link
Contributor Author

@Shimanshu83 assigned, thanks for contributing.

@lucifercr07
Copy link
Contributor Author

@Shimanshu83 please let us know if any more details required on this.

@Shimanshu83
Copy link
Contributor

Sure

@lucifercr07
Copy link
Contributor Author

@Shimanshu83 if you're facing some issue or need more details, let me know would be happy to jump on a call and resolve. This issue is blocking UI integration hence we'd like to start work on this.

@Shimanshu83
Copy link
Contributor

Hey, was bit busy on working days, but will look into this today only.

@lucifercr07
Copy link
Contributor Author

Sure, just to note when implementing the cron, we should have a key stored in DiceDB which would hold the time since last cleanup and UI would use that.

@Shimanshu83
Copy link
Contributor

After each cron job trigger, we should delete all the keys and then add a last_time_clean_up key to store the timestamp of the most recent trigger. Does that sound correct?

@lucifercr07
Copy link
Contributor Author

@Shimanshu83 yes please raise the draft PR once.

@Shimanshu83
Copy link
Contributor

Ok will raise it in sometime.

@Shimanshu83
Copy link
Contributor

@lucifercr07 saving a last_clean_up cron at dicedb is I think not a good idea since other users can change that key or delete it. Can we save it as a global variable.

@lucifercr07
Copy link
Contributor Author

@Shimanshu83 We can name it uniquely so that it can't be deleted, we should try avoid global variables from what I understand. Also if the server or application crashes variable won't be persisted.

@Shimanshu83
Copy link
Contributor

So I am using flushdb command to delete all the keys at once. Is there any observations on that. We can recreate that last cleanup key once the flushdb command is executed.

@lucifercr07
Copy link
Contributor Author

lucifercr07 commented Oct 12, 2024

We need to have a way so that we don't flush all keys, ratelimit keys are also persisted. We need to have internal keys with prefix including hash of some sort to have a fixed naming pattern and avoid cleaning these up. It can be taken as part of separate issue.

@aasifkhan7
Copy link
Contributor

aasifkhan7 commented Oct 14, 2024

what do you guys mean by global variables @lucifercr07 @Shimanshu83 ? where do they get stored?

@lucifercr07
Copy link
Contributor Author

@Shimanshu83 change has been merged as part of #43, I see you've raised your PR recently, please resolve conflicts. I believe we can add below comment changes and integration test as part of your PR, let me know your thoughts:

  1. Integrate Ratelimiter Headers and Cron Cleanup Timer with Playground UI alloy#66 (comment)
  2. Integrate Ratelimiter Headers and Cron Cleanup Timer with Playground UI alloy#66 (comment)

@Shimanshu83
Copy link
Contributor

@lucifercr07 since above two comments is already assigned to some one. I can do one thing I can raise new pr with the integration test for the implementation of clean up cron functionality which is currently merged in master.

@lucifercr07
Copy link
Contributor Author

@Shimanshu83 sure, let me know if you want me to create an issue for it.

@Shimanshu83
Copy link
Contributor

Yes @lucifercr07 create a new issue and tag me.

@lucifercr07
Copy link
Contributor Author

@Shimanshu83 created: #50 , please comment once to assign.

@lucifercr07
Copy link
Contributor Author

Closing merged as part of #43

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers stack -- go
Projects
None yet
Development

No branches or pull requests

5 participants