-
Notifications
You must be signed in to change notification settings - Fork 552
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
rptest: Fix cloud_retention_test #16943
Conversation
new failures in https://buildkite.com/redpanda/redpanda/builds/45817#018e1ab0-f9ca-4570-8518-c507ae4dd388:
new failures in https://buildkite.com/redpanda/redpanda/builds/45817#018e1ab0-f9c7-448a-8c27-72904d8ba75c:
|
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.
the throughput parameter was passed to the parameter that limited max number of messages.
omg. nice catch!
looks like we may need to tweak a few tunables to get it to pass? |
@@ -188,14 +189,15 @@ def check_total_size(include_below_start_offset): | |||
msg_size, | |||
readers=3, | |||
loop=True, | |||
max_msgs=max_read_msgs, | |||
max_throughput_mb=max_read_msgs, |
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.
It still seems a little strange that we're using a number of messages as a rate in mb. Maybe this should be max_consume_rate_mb?
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.
yep, fixed that
no idea why it used like that before
Fix retention test. Previously, the throughput parameter was passed to the parameter that limited max number of messages. Because of that the test was only able to consume fixed number of messages in CDT. This commit fixes this and improves few other aspects (error handling and logging).
ea5a967
to
042e4c2
Compare
ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/45863#018e1dc2-86ab-4644-a50b-e4ff9156c81d |
Thanks for the fix, @Lazin ! |
/backport v23.3.x |
/backport v23.2.x |
Failed to create a backport PR to v23.2.x branch. I tried:
|
Fix retention test. Previously, the throughput parameter was passed to the parameter that limited max number of messages. Because of that the test was only able to consume fixed number of messages in CDT. This commit fixes this and improves few other aspects (error handling and logging).
Fixes #15893
Backports Required
Release Notes