Skip to content

Fix test, disable expiration until empty buckets are formed#12689

Merged
oranagra merged 1 commit intoredis:unstablefrom
hpatro:stabilize_expiry_test_2
Oct 24, 2023
Merged

Fix test, disable expiration until empty buckets are formed#12689
oranagra merged 1 commit intoredis:unstablefrom
hpatro:stabilize_expiry_test_2

Conversation

@hpatro
Copy link
Contributor

@hpatro hpatro commented Oct 23, 2023 •
edited by oranagra
Loading

Test failure on freebsd CI:
https://github.com/redis/redis/actions/runs/6607001072/job/17943822780#step:4:8901

*** [err]: expire scan should skip dictionaries with lot's of empty buckets in tests/unit/expire.tcl
  scan didn't handle slot skipping logic.

Observation:

expiry of keys might happen before the empty buckets are formed and won't help with the expiry skip logic validation.

Solution:

Disable expiration until the empty buckets are formed.

Additional testing:

Added some wait between key deletion and observed the test was failing prior to this change.

        # delete data to have lot's (99%) of empty buckets (slot 12182 should be skipped)
        for {set j 1} {$j <= 99} {incr j} {
            after 100
            r del "{foo}$j"
        }

@hpatro hpatro mentioned this pull request Oct 23, 2023
Merged
@hpatro hpatro requested a review from oranagra October 23, 2023 20:26
@hpatro hpatro force-pushed the stabilize_expiry_test_2 branch from 88cf181 to 384c707 Compare October 23, 2023 20:29
@oranagra oranagra changed the title Disable expiration until empty buckets are formed Fix test, disable expiration until empty buckets are formed Oct 24, 2023
oranagra
oranagra approved these changes Oct 24, 2023
View reviewed changes
Copy link
Member

@oranagra oranagra left a comment

Choose a reason for hiding this comment

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

thanks.
yes, the freebsd CI is extremely slow.

@oranagra oranagra merged commit 3fac869 into redis:unstable Oct 24, 2023
@enjoy-binbin
Copy link
Contributor

enjoy-binbin commented Oct 25, 2023 •
edited
Loading

https://github.com/redis/redis/actions/runs/6633917815/job/18022505154

*** [err]: expire scan should skip dictionaries with lot's of empty buckets in tests/unit/expire.tcl
Keys did not actively expire.

i am debuging it, and if we add a [r keys *] before r debug set-active-expire 1, we can reproduce the failure locally.

so i guess in the daily, it fail because this code (running dbBuckets):

    /* Show some info about non-empty databases */
    if (server.verbosity <= LL_VERBOSE) {
        run_with_period(5000) {
            for (j = 0; j < server.dbnum; j++) {
                long long size, used, vkeys;

                size = dbBuckets(&server.db[j], DB_MAIN);
                used = dbSize(&server.db[j], DB_MAIN);
                vkeys = dbSize(&server.db[j], DB_EXPIRES);
                if (used || vkeys) {
                    serverLog(LL_VERBOSE,"DB %d: %lld keys (%lld volatile) in %lld slots HT.",j,used,vkeys,size);
                }
            }
        }
    }

@hpatro
Copy link
Contributor Author

hpatro commented Oct 25, 2023

Thanks @enjoy-binbin. While fixing bug #12694, we discovered dbBuckets was taking more than 50% of the CPU time due to info stats being invoked for active_defrag_running stat every 100ms.

@roshkhatri / I are looking into dbBuckets performance regression/impact.

@hpatro
Copy link
Contributor Author

hpatro commented Oct 26, 2023

Here is a fix to improve performance of dbBuckets method #12697

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@oranagra oranagra oranagra approved these changes

+1 more reviewer

@enjoy-binbin enjoy-binbin enjoy-binbin approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@hpatro @enjoy-binbin @oranagra