-
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
Pass request-params as is in supported operations #649
Pass request-params as is in supported operations #649
Conversation
For all operations the `request-params` property, allow passing any parameter without requiring that its explicitly supported by the respective Elasticsearch Py API. Closes elastic#302
Note: this PR requires some changes in the tracks repo as well as github.com/elastic/rally-eventdata-track to ensure there are no uses of json booleans with request-params. Therefore marking it as WiP for now. |
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.
Looks fine for the most part. I left a few specific comments and I wonder whether it would make sense to add a test that we can pass arbitrary parameters for queries.
docs/track.rst
Outdated
* ``request-params`` (optional): A structure containing arbitrary request parameters. The supported parameters names are documented in the `Python ES client API docs <http://elasticsearch-py.readthedocs.io/en/master/api.html#elasticsearch.Elasticsearch.search>`_. Parameters that are implicitly set by Rally (e.g. `body` or `request_cache`) are not supported (i.e. you should not try to set them and if so expect unspecified behavior). | ||
* ``request-params`` (optional): A structure containing arbitrary request parameters. The supported parameters names are documented in the `Search URI Request docs <https://www.elastic.co/guide/en/elasticsearch/reference/current/search-uri-request.html#_parameters_3>`_. | ||
|
||
NOTE: |
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.
Should we use the RST formatting approach for notes here? I.e.:
.. note::
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.
I didn't elect to use .. note::
because I was unsure if it can be indented (the note applies for request-params
only). It appears this is possible, so I addressed it in 70639ce
and add a test case to ensure that if cache has been set it is properly propagated inside request-params.
esrally/driver/runner.py
Outdated
@@ -593,7 +593,7 @@ def __call__(self, es, params): | |||
def request_body_query(self, es, params): | |||
request_params = params.get("request-params", {}) | |||
if "cache" in params: | |||
request_params["request_cache"] = params["cache"] | |||
request_params["request_cache"] = str(params["cache"]).lower() if isinstance(params["cache"], bool) else params["cache"] |
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.
simplify the right hand side to str(params["cache"]).lower()
?
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.
Addressed in b0d6c41
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.
LGTM
For all operations the
request-params
property, allow passing anyparameter without requiring that its explicitly supported by the
respective Elasticsearch Py API.
Closes #302