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 AOFRW limit test occasional failures on slower machines #10164

Merged
merged 2 commits into from
Jan 24, 2022
Merged

Fix AOFRW limit test occasional failures on slower machines #10164

merged 2 commits into from
Jan 24, 2022

Conversation

chenyang8094
Copy link
Collaborator

@chenyang8094 chenyang8094 commented Jan 24, 2022

issue link: #9788 (comment)

Comment on lines 1114 to 1116
# Write more than 1KB data to trigger AOFRW
for {set j 0} {$j < 1024} {incr j} {
r set [expr rand()] [expr rand()]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this isn't very efficient (many small keys which unknown size, and also no pipeline).
why not just a single SETRANGE x 1024 1?
or if we want many small keys populate 1024

Copy link
Collaborator Author

@chenyang8094 chenyang8094 Jan 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you misunderstood the mechanism of AOFRW's auto-triggering, IIUC redis compares the number of bytes written to the AOF file, not the number of keys in redis.

if (server.aof_state == AOF_ON &&
            !hasActiveChildProcess() &&
            server.aof_rewrite_perc &&
            server.aof_current_size > server.aof_rewrite_min_size &&
            !aofRewriteLimited())
{
     AOFRW();
}

Copy link
Member

@oranagra oranagra Jan 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ohh, right (not misunderstood, just forgot for a moment).
so scrap the SETRANGE suggestion.
it can be r SET x [string repeat x 1024], or populate 1024 (to get multiple keys)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

populate 1024 doesn't work,because it won't be written to AOF.
r SET x [string repeat x 1024] is a good suggestion.

p.s. I found that when changing auto-aof-rewrite-min-size to 1kb, AOFRW will be triggered automatically (because a lot of data was written in the previous test, causing server.aof_current_size to exceed 1KB). So at the beginning of the test, I first flush the data and executed an AOFRW to ensure that server.aof_current_size is 0.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok.. but not sure why populate won't work. it's a function (not DEBUG POPULATE).

# Write num keys with the given key prefix and value size (in bytes). If idx is
# given, it's the index (AKA level) used with the srv procedure and it specifies
# to which Redis instance to write the keys.
proc populate {num {prefix key:} {size 3} {idx 0}} {
    set rd [redis_deferring_client $idx]
    for {set j 0} {$j < $num} {incr j} {
        $rd set $prefix$j [string repeat A $size]
    }
    for {set j 0} {$j < $num} {incr j} {
        $rd read
    }
    $rd close
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ohh, I did understand it was debug populate, you are right.

@oranagra oranagra added this to Needs triage in Triage via automation Jan 24, 2022
@oranagra oranagra moved this from Needs triage to Awaits merge in Triage Jan 24, 2022
@oranagra oranagra merged commit 6dc3f09 into redis:unstable Jan 24, 2022
Triage automation moved this from Awaits merge to Done Jan 24, 2022
panjf2000 pushed a commit to panjf2000/redis that referenced this pull request Feb 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants