ccmlib/node: check both pending and active tasks when waiting for compactions #611
Conversation
…pactions - Rename Node._parse_pending_tasks to Node._parse_tasks - Count all tasks, pending and active - Allow searching for tasks based on only keyspace - Update and refactor test to allow more varied cases
|
How does backporting work in this repo? I see the labels but no backport PRs. |
we don't have mergify setup for this one, labels are used. |
Ok, so I assume this needs to go in 6.0, 6.1 and 2024.2 ? |
tests/test_internal_functions.py
Outdated
| ]) | ||
| @pytest.mark.parametrize("test_case", test_cases, ids=[tc["id"] for tc in test_cases]) | ||
| def test_parse_tasks(test_case): | ||
| output = test_case["output"] |
There was a problem hiding this comment.
Use multiple arguments, instead of test_case
There was a problem hiding this comment.
Why not change the test_cases directly to have pytest.param?
There was a problem hiding this comment.
Then test_cases would look like
test_cases = [
pytest.param(
textwrap.dedent("""\
pending tasks: 6
- system_schema.tables: 1
- system_schema.columns: 2
- keyspace1.standard1: 3
\
"""),
[
("system_schema", "tables", 1),
("system_schema", "columns", 2),
("system_schema", None, 3),
("keyspace1", "standard1", 3),
("keyspace1", None, 3),
(None, None, 6),
("keyspace1x", None, 0),
("keyspace1x", "table1x", 0),
],
id="only_pending"
),
...
]Which I don't know if it's better.
I like having the more explicit naming of everything, because pytest.param relies of order.
There was a problem hiding this comment.
That actually looks much better to me, it is exactly the structure the pytest.parametrize should have in my opinion and thus making it easier to understand in my opinion. But I might be in minority here, so I will leave it out to others to decide
There was a problem hiding this comment.
Both are o.k. from my POV
It's complex regardless of how you might present it.
Just a reference for issue in pytest about trying to introduce kwargs support for pytest.param, which never happened (but external plugging that supply something like it)
pytest-dev/pytest#9216 (comment)
|
Could you please link issues that should be fixed by this? |
The initial issue is https://github.com/scylladb/scylla-dtest/issues/4702. Updated description to mention it. |
cezarmoise
added
Backport-6.0
Currently, when using
wait_for_compactions, only pending tasks are considered. If there are active compaction tasks, the function will return anyway and this causes flakyness in tests, as sometimes the active task finishes before the next operation, but sometimes it doesn't.closes: #610
refs: https://github.com/scylladb/scylla-dtest/issues/4702