-
Notifications
You must be signed in to change notification settings - Fork 6.8k
[core] Inject reorder_wait_seconds for scheduling queue test #54404
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
Conversation
Signed-off-by: dayshah <[email protected]>
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.
Pull Request Overview
This PR makes the reorder_wait_seconds for actor scheduling queues injectable so tests can run with a reduced timeout (1s instead of the default 30s).
- Exposed
reorder_wait_secondsas a constructor parameter inActorSchedulingQueue - Updated
TaskReceiverto pass the config value into the new queue constructor - Modified all scheduling queue tests to inject a 1-second reorder wait
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/ray/core_worker/transport/task_receiver.cc | Passes actor_scheduling_queue_max_reorder_wait_seconds() into ActorSchedulingQueue |
| src/ray/core_worker/transport/actor_scheduling_queue.h | Added reorder_wait_seconds parameter and member |
| src/ray/core_worker/transport/actor_scheduling_queue.cc | Updated constructor signature and initializer list |
| src/ray/core_worker/test/scheduling_queue_test.cc | Injects a 1-second reorder_wait_seconds in tests |
Comments suppressed due to low confidence (2)
src/ray/core_worker/transport/actor_scheduling_queue.h:49
- [nitpick] Add or update the constructor doc comment above this line to describe the new
reorder_wait_secondsparameter, so callers know its purpose and units.
int64_t reorder_wait_seconds);
src/ray/core_worker/test/scheduling_queue_test.cc:340
- [nitpick] Re-add a comment explaining why
io_service.run()is invoked here (e.g., "// immediately triggers the reordered-task timeout") to preserve test intent clarity.
io_service.run();
…ject#54404) This scheduling queue unit test would take >30 seconds before because the default value of reorder_wait_seconds is 30. Now just injecting it in one level higher so can inject it into the test as 1 second. Signed-off-by: dayshah <[email protected]> Signed-off-by: doyoung <[email protected]>
…ject#54404) This scheduling queue unit test would take >30 seconds before because the default value of reorder_wait_seconds is 30. Now just injecting it in one level higher so can inject it into the test as 1 second. Signed-off-by: dayshah <[email protected]> Signed-off-by: doyoung <[email protected]>
…ject#54404) This scheduling queue unit test would take >30 seconds before because the default value of reorder_wait_seconds is 30. Now just injecting it in one level higher so can inject it into the test as 1 second. Signed-off-by: dayshah <[email protected]> Signed-off-by: ChanChan Mao <[email protected]>
…ject#54404) This scheduling queue unit test would take >30 seconds before because the default value of reorder_wait_seconds is 30. Now just injecting it in one level higher so can inject it into the test as 1 second. Signed-off-by: dayshah <[email protected]> Signed-off-by: Douglas Strodtman <[email protected]>
Why are these changes needed?
This scheduling queue unit test would take >30 seconds before because the default value of reorder_wait_seconds is 30. Now just injecting it in one level higher so can inject it into the test as 1 second.