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

fix: only insert historical values if changed #1574

Merged
merged 1 commit into from
Jul 30, 2024
Merged

fix: only insert historical values if changed #1574

merged 1 commit into from
Jul 30, 2024

Conversation

dinhani-cw
Copy link
Contributor

@dinhani-cw dinhani-cw commented Jul 30, 2024

PR Type

Bug fix


Description

  • Modified the zadd command in RedisPermanentStorage to use the NX option, ensuring that only new elements are added to the sorted set.
  • Replaced the direct zadd call with a custom Redis command using redis::cmd to incorporate the NX option.

Changes walkthrough 📝

Relevant files
Bug fix
redis_permanent.rs
Ensure historical values are only inserted if changed in Redis

src/eth/storage/redis/redis_permanent.rs

  • Modified the zadd command to use the NX option to ensure only new
    elements are added.
  • Replaced the direct zadd call with a custom Redis command using
    redis::cmd.
  • +4/-4     

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    @dinhani-cw dinhani-cw enabled auto-merge (squash) July 30, 2024 22:23
    Copy link

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Key issues to review

    Possible Bug
    The cmd.exec(&mut conn) call on line 140 should be checked for proper error handling. Ensure that the command execution is correctly monitored and logged.

    Logging Improvement
    The error logging on line 142 should include the key and value as tracing fields for better context.

    Copy link

    github-actions bot commented Jul 30, 2024

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Best practice
    Use cmd.query(&mut conn) instead of cmd.exec(&mut conn) for better error handling and type safety

    Instead of using cmd.exec(&mut conn), consider using cmd.query(&mut conn) which
    returns a Result and can provide better error handling and type safety.

    src/eth/storage/redis/redis_permanent.rs [140]

    -let zadd: RedisVoid = cmd.exec(&mut conn);
    +let zadd: Result<RedisVoid, redis::RedisError> = cmd.query(&mut conn);
     
    Suggestion importance[1-10]: 9

    Why: This suggestion improves error handling and type safety, which are crucial aspects in a storage system. Using cmd.query(&mut conn) returns a Result, making the code more robust and easier to debug.

    9
    Maintainability
    Extract the ZADD command creation into a separate function for better readability and maintainability

    To improve readability and maintainability, consider extracting the ZADD command
    creation into a separate function.

    src/eth/storage/redis/redis_permanent.rs [137-138]

    -let mut cmd = redis::cmd("ZADD");
    -cmd.arg(key).arg("NX").arg(score).arg(value);
    +fn create_zadd_command(key: &str, score: f64, value: &str) -> redis::Cmd {
    +    let mut cmd = redis::cmd("ZADD");
    +    cmd.arg(key).arg("NX").arg(score).arg(value);
    +    cmd
    +}
     
    +let cmd = create_zadd_command(key, score, value);
    +
    Suggestion importance[1-10]: 7

    Why: This suggestion enhances code readability and maintainability by encapsulating the ZADD command creation in a separate function. This makes the main logic cleaner and easier to understand, though it is a minor improvement.

    7

    @dinhani-cw dinhani-cw merged commit 83ea6e0 into main Jul 30, 2024
    34 checks passed
    @dinhani-cw dinhani-cw deleted the fix-redis branch July 30, 2024 22:29
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    None yet
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    1 participant