rptest: assert !_stopped in KgoVerifierServices::wait() implementations
#27298
+37
−40
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Retry command for Build#71026please wait until all jobs are finished before running the slash command |
WillemKauf
force-pushed
the
stop_kgo_checks
branch
from
f60591c to
9c60273
Compare
Retry command for Build#71031please wait until all jobs are finished before running the slash command |
`KgoVerifierService` objects should be waited upon and then stopped, not the other way around.
WillemKauf
force-pushed
the
stop_kgo_checks
branch
from
9c60273 to
5baac73
Compare
WillemKauf
changed the title
rptest: catch bad testsrptest: assert !_stopped in KgoVerifierServices::wait() implementations
Retry command for Build#71034please wait until all jobs are finished before running the slash command |
CI test resultstest results on build#71034
test results on build#71074
test results on build#71084
test results on build#71154
test results on build#71191
test results on build#71239
test results on build#71250
|
WillemKauf
force-pushed
the
stop_kgo_checks
branch
from
5baac73 to
58a8d45
Compare
WillemKauf
force-pushed
the
stop_kgo_checks
branch
from
58a8d45 to
50613bd
Compare
WillemKauf
force-pushed
the
stop_kgo_checks
branch
from
50613bd to
422a64b
Compare
|
/ci-repeat 1 |
|
/ci-repeat 1 |
|
/ci-repeat 1 |
|
/ci-repeat 1 |
nvartolomei
previously approved these changes
Aug 21, 2025
| f"Can't wait {self.who_am_i()}. It was already stopped." | ||
| f" You can either stop() a service or wait() and then stop() it" | ||
| f" but not the other way around.") | ||
| assert not self._stopped, f"Can't wait {self.who_am_i()}. It was already stopped. You can either stop() a service or wait() and then stop() it but not the other way around." |
There was a problem hiding this comment.
can probably contribute this to ducktape itself
| @@ -100,6 +105,7 @@ def test_moving_single_partition_under_load(self, replication_factor): | |||
| self._do_move_and_verify(t, partition, 600) | |||
|
|
|||
| self.verify(topic.name) | |||
| self.cleanup() | |||
There was a problem hiding this comment.
verify already called wait(). cleanup is not necessarily needed then, right?
| def verify(self, topic_name, msg_size, consumers): | ||
| self.producer.wait() | ||
|
|
||
| # Await the consumer that is reading only the subset of data that | ||
| # was written before it started. | ||
| self.consumer.wait() | ||
| self.consumer.stop() |
There was a problem hiding this comment.
this should be a noop, right? wait encapsulates a stop
| @@ -169,3 +174,4 @@ def test_io_stress(self, cloud_storage_type, segment_size, | |||
| self._create_initial_topics() | |||
|
|
|||
| self._workload(segment_size, interval_uploads) | |||
| self.cleanup() | |||
There was a problem hiding this comment.
ditto: why do we need this? it is all dead code to me
WillemKauf
force-pushed
the
stop_kgo_checks
branch
from
422a64b to
d955baa
Compare
Retry command for Build#71154please wait until all jobs are finished before running the slash command |
WillemKauf
force-pushed
the
stop_kgo_checks
branch
from
d955baa to
db0b138
Compare
|
Rebased |
mmaslankaprv
previously approved these changes
Aug 22, 2025
nvartolomei
reviewed
Aug 22, 2025
| @@ -490,7 +492,6 @@ def calculate_total_learners_gap() -> int | None: | |||
|
|
|||
| self.logger.info(f"decommissioning node: {to_decommission_id}", ) | |||
| self._decommission(to_decommission_id) | |||
| self.producer.wait() | |||
There was a problem hiding this comment.
why is wait removed from there? this changes what the test does no? previously the test waited for consumer to finish before proceeding. but now?
There was a problem hiding this comment.
assert not self._stopped, f"Can't wait {self.who_am_i()}. It was already stopped. You can either stop() a service or wait() and then stop() it but not the other way around." AssertionError: Can't wait KgoVerifierProducer-0-140548171042240. It was already stopped. You can either stop() a service or wait() and then stop() it but not the other way around.
Oh, that's why I removed it.
There was a problem hiding this comment.
The second producer.wait() call in verify() will fail with the new assert if this wait() is left here.
It is also unnecessary for the test as written.
WillemKauf
force-pushed
the
stop_kgo_checks
branch
from
db0b138 to
eb3daa3
Compare
nvartolomei
previously approved these changes
Aug 22, 2025
Retry command for Build#71239please wait until all jobs are finished before running the slash command |
The second `producer.wait()` call in `verify()` will fail with the new assert if this `wait()` is left here. It is also unnecessary for the test as written.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
KgoVerifierServiceobjects should be waited upon and then stopped, not the other way around.Backports Required
Release Notes