Check redis key exists on delete#87
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @TYashodaKrishna, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request refines the Redis cache deletion logic to improve efficiency and robustness. It introduces a preliminary check for key existence before attempting a delete operation, ensuring that deletion commands are only issued when necessary. This change aims to optimize interactions with the Redis cache by avoiding redundant operations.
Highlights
- Conditional Redis Key Deletion: The
Deletefunction in the Redis cache implementation now includes a preliminary check to determine if a key exists before attempting to delete it. This ensures that the deletion command is only sent to Redis if the key is present or if there was an issue checking its existence.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
The pull request aims to prevent an error when deleting a non-existent key from Redis by first checking if the key exists. However, the current implementation introduces a critical issue: due to a bug in the c.exists function it relies on, the Delete function will no longer remove any keys. Additionally, this approach introduces a race condition and is less efficient. I've provided a review comment with a suggested code change that correctly and efficiently implements the desired behavior by using the atomic DEL command directly.
| exists, err := c.exists(cx, key) | ||
| if exists || err != nil { | ||
| err = c.delete(cx, key) | ||
| if err != nil { | ||
| c.logger.Error( | ||
| "failed to delete data from redis", | ||
| log.String("key", key), | ||
| log.Error(err), | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
This change introduces a check-then-act pattern (EXISTS then DEL) which has several issues:
- Race Condition: It's not atomic. Another client could modify the key between the
EXISTSandDELcalls, leading to unpredictable behavior. - Inefficiency: It requires an extra network round-trip for each deletion.
- Broken Logic: The
c.existsfunction appears to be implemented incorrectly and will always returnfalsefor existing keys. This means thisDeletefunction will never actually delete a key unlessc.existsitself returns an error, breaking the functionality.
The DEL command in Redis is atomic. A much simpler and more correct approach is to call DEL directly and check for an error. The go-redis client's Del() method does not return an error if the key doesn't exist, which aligns perfectly with the Cache.Delete interface contract ("doesn't do anything if key is not found").
I suggest replacing the logic with a direct call to c.cc.Del().
if err := c.cc.Del(cx, key).Err(); err != nil {
c.logger.Error(
"failed to delete data from redis",
log.String("key", key),
log.Error(err),
)
}
There was a problem hiding this comment.
@TYashodaKrishna fix this. This is critical. The two operations aren't atomic and will fail.
| exists, err := c.exists(cx, key) | ||
| if exists || err != nil { | ||
| err = c.delete(cx, key) | ||
| if err != nil { | ||
| c.logger.Error( | ||
| "failed to delete data from redis", | ||
| log.String("key", key), | ||
| log.Error(err), | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
@TYashodaKrishna fix this. This is critical. The two operations aren't atomic and will fail.
2 participants
No description provided.