-
Notifications
You must be signed in to change notification settings - Fork 313
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
Bulk action refresh param #1743
Conversation
Approach looks fine to me, but curious how you intend to handle more complex configurations where want to change the |
I am not too concerned with changing the Does it make more sense to change the possible values to strings only? Like:
This would leave the door open for a future parameter value like |
The latest commit uses this model. It can be tested against serverless with the track in elastic/rally-tracks#425.
|
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.
Left some comments, but it otherwise LGTM.
esrally/driver/runner.py
Outdated
if params["refresh"] in valid_refresh_values: | ||
bulk_params["refresh"] = params["refresh"] | ||
else: | ||
self.logger.info( |
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.
Sorry for my last comment, I see now what you're checking for. I think both INFO
and WARNING
aren't enough this case, and we should probably just error out because of an invalid value - otherwise we risk returning misleading results and the corresponding log message is buried deep in the file somewhere.
Maybe we can error out similar to how it's done here?:
rally/esrally/driver/runner.py
Lines 2609 to 2612 in b1a822c
if op_type not in self.supported_op_types: | |
raise exceptions.RallyAssertionError( | |
f"Unsupported operation-type [{op_type}]. Use one of [{', '.join(self.supported_op_types)}]." | |
) |
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.
We could probably also just lean on the client itself to return an error, if you pass something in that's not valid does it return anything meaningful?
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.
Left one minor comment around unsupported values, but no need for another review cycle. LGTM.
This commit adds support for the bulk
?refresh
query parameter to address feedback for a new workload for serverless. Possible values forrefresh
aretrue
(?refresh=true
) for async,false
(?refresh=false
), andwait_for
(?refresh=wait_for
) for sync, consistent with https://www.elastic.co/guide/en/elasticsearch/reference/current/docs-bulk.html#bulk-refresh.