add client ramp up feature. #716
Conversation
Signed-off-by: Rishabh Singh <[email protected]>
rishabh6788
requested review from
IanHoang,
gkamat,
beaioun,
cgchinmay,
VijayanB and
OVI3D0
as code owners
rishabh6788
force-pushed
the
main
branch
4 times, most recently
from
aaa923f to
aeeba9c
Compare
| "ramp-up-time-period": { | ||
| "type": "integer", | ||
| "minimum": 0, | ||
| "description": "Defines the time period in seconds to gradually increase the number of clients." | ||
| }, |
There was a problem hiding this comment.
Will the OSB workloads also need to be updated to accommodate this change?
There was a problem hiding this comment.
@rishabh6788 Have left a few comments but could we revise the PR to not include the additional formatting changes? Seems like a lot of it was introduced by a lint checker from the code editor. It's hard to determine all that has changed.
| // For format details, see https://aka.ms/devcontainer.json. For config options, see the | ||
| // README at: https://github.com/devcontainers/templates/tree/main/src/python | ||
| { | ||
| "name": "Python 3", |
There was a problem hiding this comment.
For users unfamiliar with dev cointainers, might be good to add some comments at the top for what scenarios and how this might be used?
| self.logger.error( | ||
| "Main worker_coordinator received a fatal indication from load generator (%s). Shutting down.", poisonmsg.details) |
There was a problem hiding this comment.
A lot of these changes in worker_coordinator.py are new lines in or reformatting existing lines. Are these intentional or was this done with an automated tool? We should minimize changes that aren't related to the proposed feature
| @@ -2009,7 +2148,7 @@ def requires_time_period_schedule(task, task_runner, params): | |||
|
|
|||
|
|
|||
| class ScheduleHandle: | |||
| def __init__(self, task_name, sched, task_progress_control, runner, params): | |||
| def __init__(self, task_allocation, sched, task_progress_control, runner, params): | |||
| """ | |||
| Creates a generator that will yield individual task invocations for the provided schedule. | |||
|
|
|||
There was a problem hiding this comment.
The docstring should also be updated to reflect the change from task_name to task_allocation
:param task_name: The name of the task for which the schedule is generated.
| @@ -1941,7 +2075,7 @@ def clients(self): | |||
|
|
|||
| # Runs a concrete schedule on one worker client | |||
| # Needs to determine the runners and concrete iterations per client. | |||
| def schedule_for(task, client_index, parameter_source): | |||
| def schedule_for(task_allocation, parameter_source): | |||
| """ | |||
| Calculates a client's schedule for a given task. | |||
|
|
|||
There was a problem hiding this comment.
Same as in ScheduleHandler, docstrings should be updated to replace task with task_allocation and remove client_index
Description
As mentioned in #713 this PR introduces a new task property,
ramp-up-time-period. When provided, opensearch-benchmark will gradually scale up clients based on the time-period and number of clients provided.Issues Resolved
#713
Testing
[Describe how this change was tested]
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.