-
Notifications
You must be signed in to change notification settings - Fork 577
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
tests: fix test_changing_topic_retention_with_restart #5331
Conversation
a2c0ab3
to
f35e164
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks fine to me. I'll leave it to someone with more in depth knowledge of the tests to approve though. Is the 5% jitter something that you figured out empirically?
Nope, it's this constant in the source:
While I was updating this I added a reference to the comment in the test. |
This test was assuming that segment sizes apply exactly, but they are actually subject to a +/- 5% jitter. To reliably remove the expected number of segments, we must set our retention bytes to 5% less than the amount we really expect to retain. Fixes redpanda-data#2406
f35e164
to
20949e3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
CI failures are two issues from partition movement cancellation: |
@jcsp looks like this will backport to v22.1.x, v21.11.x |
Cover letter
This test was assuming that segment sizes apply exactly,
but they are actually subject to a +/- 5% jitter. To reliably
remove the expected number of segments, we must set our retention
bytes to 5% less than the amount we really expect to retain.
Fixes #2406
Release notes