-
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
Allow to specify static responses #1234
Allow to specify static responses #1234
Conversation
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.
Thanks! This is a very useful PR for stubbing ES responses easily.
I left a few questions/suggestions.
# pylint: disable=unused-variable | ||
def f(p): | ||
return True | ||
return f |
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.
alternatively we could replace the closures with more Pythonic (IMHO? :) ) lambdas e.g.
return lambda f: True
or for startswith
:
def startswith(path_pattern):
return lambda f: f.startswith(path_pattern)
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 implemented this with lambdas earlier but then got a PEP-8 violation warning. :)
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.
LOL whaaaaaat?
return f | ||
|
||
@staticmethod | ||
def endswith(path_pattern): |
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.
One thought, we could enhance it in the future and support simple shell like patterns like *_snapshot/frozen*
easily with https://docs.python.org/3/library/fnmatch.html. This could be quite useful.
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.
Good idea. Before we enhance it, we should probably run microbenchmarks to get a better grasp of the overhead though because we're on the hot code path.
docs/command_line_reference.rst
Outdated
esrally race --track=geonames --challenge=append-no-conflicts-index-only --pipeline=benchmark-only --skip-rest-api-check --distribution-version=8.0.0 --client-options="static_responses:'responses.json'" | ||
|
||
.. note:: | ||
Use ``--skip-rest-api-check`` to ensure that Rally skips any probing requests that are not directly related to the benchmark (e.g. checks whether the cluster is up). |
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.
Could we automatically skip rest api checks when static_responses
is defined?
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.
Thanks for the idea! I've pushed a change in af77d1b. In that change I've also:
- Turned
--skip-rest-api-check
into an undocumented flag again. With the changes in this PR it's also likely that we won't need that flag at all in the future but I'd like to keep it for now. - Improved logging
- Avoided issuing any additional requests (probing for cluster version, telemetry devices) when static responses are enabled.
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 suggestion (up to you to implement), but LGTM. This is very useful!
|
||
Save the above responses as ``responses.json`` and execute a benchmark as follows:: | ||
|
||
esrally race --track=geonames --challenge=append-no-conflicts-index-only --pipeline=benchmark-only --distribution-version=8.0.0 --client-options="static_responses:'responses.json'" |
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.
It took me a few seconds to realize that while specifying a pipeline
it won't really target any ES. One might even use the default pipeline, ES will get launched but won't be used.
Should we clarify with a note here or above in the client-options
section that when static_responses:'file'
is used, the use should also specify the benchmark-only
pipeline and the distribution-version
as Rally doesn't have a way to derive the version automatically as it'd normally do.
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.
Thanks! I've added a note in 37c0900.
With this commit we add the ability to short-circuit Rally's networking layer
and instead of targeting a real cluster, Rally will return static user-defined
responses. This simplifies client-side analysis for bottlenecks or load driver
behavior in general which previously required to setup a stub server (e.g.
nginx)