roachtest: add groups to task manager #135831
Conversation
|
This change is |
herkolategan
force-pushed
the
hbl/roachtest-task-groups
branch
2 times, most recently
from
73c9af6 to
50f6ae1
Compare
|
|
||
| select { | ||
| case <-done: | ||
| case <-time.After(30 * time.Second): |
There was a problem hiding this comment.
I'm curious if this is good practice to have a timeout in a test, or rather wait indefinitely and let a "global test timeout" happen (if something broke the test)?
There was a problem hiding this comment.
I'm not sure how stress/race interacts with time, but it does feel like this would break under that.
herkolategan
force-pushed
the
hbl/roachtest-task-groups
branch
2 times, most recently
from
b42d5ca to
52d7f3d
Compare
herkolategan
requested review from
nameisbhaskar and
vidit-bhat
and removed request for
a team
herkolategan
force-pushed
the
hbl/roachtest-task-groups
branch
3 times, most recently
from
50f5e59 to
326c125
Compare
| <-m.CompletedEvents() | ||
| } | ||
|
|
||
| select { |
There was a problem hiding this comment.
Would this be better suited as a WaitWithTimeout method instead? Although I took a very brief peek at monitor.wait and errgroup.wait usages and I only found two places that would use it. I'm guessing you'd have a better idea of how common this pattern is used.
There was a problem hiding this comment.
I opted to remove the timeouts. A part of me does not like that a failure will then present as a package timeout, but I think as you mentioned adding a timeout here will probably mess with stress / race. In general there is no issue here as long as the tests are passing, but a failure won't be as obvious.
|
|
||
| select { | ||
| case <-done: | ||
| case <-time.After(30 * time.Second): |
There was a problem hiding this comment.
I'm not sure how stress/race interacts with time, but it does feel like this would break under that.
Previously, a new API for managing tasks were introduced (see cockroachdb#133263). However, this did not address roachtests that want to manage groups. In an effort to replace `errgroup`, and possibly using `monitor.Go` for task management this change introduces a group provider in the task manager. A group adds the ability to wait on a subset of tasks to finish before proceeding. The task handler will still report returned errors or panics to the test framework. Informs: cockroachdb#118214 Epic: None Release note: None
This change groups the default options used by tasks and groups into a function that can be shared when creating new groups or tasks via the mixedversion test helper. The mixedversion helper now also implements `GroupProvider` to enable grouping tasks. Informs: cockroachdb#118214 Epic: None Release note: None
This change groups the default options used by tasks and groups into a function that can be shared when creating new groups or tasks via the test framework. The `Test` interface now implements `GroupProvider` as well to enable grouping tasks. Informs: cockroachdb#118214 Epic: None Release note: None
Previously, a roachtest would need to use wait groups in order to wait on tasks. The new `GroupProvider` interface now allows tests to manage groups of tasks via the framework instead of manually. Informs: cockroachdb#118214 Epic: None Release note: None
herkolategan
force-pushed
the
hbl/roachtest-task-groups
branch
from
326c125 to
9b40ff3
Compare
|
Running a roachtest TC job with tests that has some |
|
TFTR! bors r=DarrylWong |
Previously, a new API for managing tasks were introduced (see #133263). However, this did not address roachtests that want to manage groups. In an effort to replace
errgroup, andmonitor.Gofor task management, this change introduces a group provider in the task manager.A group adds the ability to wait on a subset of tasks to finish before proceeding. The task handler will still report returned errors or panics to the test framework.
Informs: #118214
Epic: None
Release note: None