Skip to content

Reset later item flag after defrag later is done #12694

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.

Sign up for GitHub

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

Jump to bottom
Merged
oranagra merged 1 commit into from
Oct 27, 2023
Merged

Reset later item flag after defrag later is done #12694

oranagra merged 1 commit into from
Oct 27, 2023

Conversation

@roshkhatri
Copy link
Contributor

@roshkhatri roshkhatri commented Oct 25, 2023

Fixing issues described in #12672, started after #11695
Related to #12674

Fixes the `defrag didn't stop' issue.

In some cases of how the keys were stored in memory defrag_later_item_in_progress was not getting reset once we finish defragging the later items and we move to the next slot. This stopped the scan to happen in the later slots and did not get defragged.

@hpatro hpatro requested review from hpatro and oranagra October 25, 2023 21:42
hpatro
hpatro approved these changes Oct 25, 2023
View reviewed changes
Copy link
Contributor

@hpatro hpatro left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This was interesting/difficult to find. Thanks @roshkhatri for diving deep.
We observed test failures for active defrag didn't stop while running these on loop. And for the failure runs we observed active_defrag_key_hits and active_defrag_key_misses weren't a multiple of dbsize. This lead to discovering that some of the slot scanning were getting skipped due to not resetting the defrag_later_item_in_progress.

@hpatro hpatro mentioned this pull request Oct 25, 2023
Merged
oranagra
oranagra approved these changes Oct 26, 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.

so can we now revert #12672?

@hpatro
Copy link
Contributor

hpatro commented Oct 26, 2023

@oranagra There were two other kind of issues were observed earlier i.e. latency higher than the threshold and frag ratio lower than the threshold. We are running them continuously to measure the variance and possibly tweak the threshold(s) a bit.

@oranagra oranagra merged commit 7d68208 into redis:unstable Oct 27, 2023
@roshkhatri
Copy link
Contributor Author

roshkhatri commented Oct 30, 2023 •
edited
Loading

@oranagra

so can we now revert #12672?

We were running the tests continuously to see the variance but they look good now, I think we can now revert those changes and enable the tests.

@oranagra
Copy link
Member

oranagra commented Oct 31, 2023

thanks. @roshkhatri can you please make a PR?
p.s. i think some of the tests (like the one for big lists) are probably useless in cluster mode and can still be skipped.

@roshkhatri roshkhatri mentioned this pull request Oct 31, 2023
Merged
oranagra added a commit that referenced this pull request Nov 2, 2023
@roshkhatri @oranagra
re-enable defrag tests in cluster mode. (#12710)
15a048d 
Reverts the skipping defrag tests in cluster mode (done in #12672.
instead it skips only some defrag tests that are relevant for cluster modes.
The test now run well after investigating and making the changes in #12674 and #12694.

Co-authored-by: Oran Agra <oran@redislabs.com>
enjoy-binbin added a commit to enjoy-binbin/redis that referenced this pull request Jul 25, 2025
@enjoy-binbin
Reset later item flag after defrag later is done (redis#12694)
78e34a0 
Fixing issues described in redis#12672, started after redis#11695
Related to redis#12674

Fixes the `defrag didn't stop' issue.

In some cases of how the keys were stored in memory
defrag_later_item_in_progress was not getting reset once we finish
defragging the later items and we move to the next slot. This stopped
the scan to happen in the later slots and did not get
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

+2 more reviewers

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

@hpatro hpatro hpatro approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

Redis 7.4
Status: Done

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@roshkhatri @hpatro @oranagra @enjoy-binbin