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

Solve issues with tracking test in external mode #9726

Merged
merged 1 commit into from Nov 2, 2021

Conversation

oranagra
Copy link
Member

@oranagra oranagra commented Nov 2, 2021

Another attempt to solve the problem described in #9722

The issue was that setting maxmemory to used_memory and expecting
eviction is insufficient, since we need to take
mem_not_counted_for_evict into consideration.

This test got broken by #9166

The issue was that setting maxmemory to used_memory and expecting
eviction is insufficient, since we need to take
mem_not_counted_for_evict into consideration.

This test got broken by redis#9166
@oranagra
Copy link
Member Author

oranagra commented Nov 2, 2021

@madolson please take a look. i'd like to merge this ASAP since it causes failures to all PRs and causes confusion.

The change that broke it is that now when the replication backlog is growing, we consider it a mem_not_counted_for_evict, and in the past it was a static pre-allocation.
The discussion that lead to the behavior change is here #9166 (comment)
We may wanna re-open that discussion, but either way i think the fix for the test in this PR is good.

@ShooterIT @soloestoy @yossigo @yoav-steinberg FYI.

Copy link
Contributor

@madolson madolson left a comment

Choose a reason for hiding this comment

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

Sure, seems reasonable.

@madolson madolson merged commit d25dc08 into redis:unstable Nov 2, 2021
@ShooterIT
Copy link
Collaborator

Thanks @oranagra i suspect my replication buffer breaks this test, but i can't find any clue
you think the replica client output buffer is more than backlog size, this part would be released gradually so there will be enough memory to store big key and doesn't evict it?

@oranagra oranagra deleted the tracking_test_wip branch November 3, 2021 05:53
@oranagra
Copy link
Member Author

oranagra commented Nov 3, 2021

@ShooterIT the reason your PR broke this test is that in the past, the replication backlog was pre-allocated, and also not considered a mem_not_counted_for_evict (i.e on one hand, the backlog creation could have triggered eviction, but on the other hand it was pre-allocated so it normally didn't do that).
But now, it is not pre-allocated, and instead it gradually grows. when it does that (grow or shrink) it is counted as used memory (not excluded from eviction), but i think it still affects this test since it was too fragile and didn't do the right math.

I.e. this test measured the amount of used memory, and then set a memory limit just above that, in order to trigger eviction. i.e. it assumed that when maxmemory==used_memory, eviction would happen. but that's not right since it needs to take mem_not_counted_for_evict into consideration too.

the reason i think this behavior change is probably ok (memory that is gradually growing and is counted for eviction, as i analyzed in #9166 (comment)), is that it cannot lead to an endless eviction feedback loop (since we only count the buffer size up to the configured limit), but i didn't foresee the possible collision with funny use cases.
For now, i think this would only be an issue for tests, not real applications, but maybe someone else can see a different issue / use case.

@oranagra
Copy link
Member Author

oranagra commented Nov 3, 2021

sorry, my previous post was a mess (forgot that the initial replication buffer up to the configured size is considered a "not counted memory", but in fact only the extra beyond the configured size is). edited the above post.

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.

None yet

3 participants