Skip to content
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

Fail Rally early if there are unused variables in track-params #688

Merged
merged 18 commits into from
May 23, 2019

Conversation

dliappis
Copy link
Contributor

@dliappis dliappis commented May 6, 2019

At the moment we are not checking if user-defined parameters in --track-params actually
do end up getting applied in any of the referenced jinja2 templates in the track, index body or referenced index templates. This can be very dangerous e.g. when trying to influence crucial benchmarking parameters that are parametrizable in our standard tracks, like number_of_shards, source_enabled etc.

Fail Rally fast if unused parameters get supplied with --track-params.

Resolves #478

This is fully working, but doesn't address variables in track / body /
index_template in a uniform way.
This WiP is fully working and catches cases when a track-param overrides
only index settings or index templates.
and add new integration tests and unit tests. Also clean up debug
statements.
@dliappis dliappis added the enhancement Improves the status quo label May 6, 2019
@dliappis dliappis added this to the 1.1.0 milestone May 6, 2019
@dliappis
Copy link
Contributor Author

dliappis commented May 6, 2019

@danielmitterdorfer I am open to suggestions as to the milestone, perhaps 1.1.0 already has too long a list of improvement to bundle this in as well.

@dliappis dliappis changed the title Fail Rally early there are unused variables in track-params Fail Rally early if there are unused variables in track-params May 6, 2019
@dliappis dliappis modified the milestones: 1.1.0, 1.1.1 May 7, 2019
Copy link
Member

@danielmitterdorfer danielmitterdorfer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did a first pass focussing mainly on the usability aspects. Wdyt?

esrally/track/loader.py Outdated Show resolved Hide resolved
esrally/track/loader.py Outdated Show resolved Hide resolved
esrally/track/loader.py Outdated Show resolved Hide resolved
esrally/track/loader.py Outdated Show resolved Hide resolved
esrally/track/loader.py Outdated Show resolved Hide resolved
esrally/track/loader.py Outdated Show resolved Hide resolved
@dliappis
Copy link
Contributor Author

I tested the workflow in vagrant, including the changes to address PR comments and suggestions for the correct parameters, and our nightlies seem to ok. I've requested another review pass.

Copy link
Member

@danielmitterdorfer danielmitterdorfer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did another pass. I really like the new error message! :) I also left a couple more comments about the implementation.

esrally/track/loader.py Outdated Show resolved Hide resolved
esrally/track/loader.py Show resolved Hide resolved
esrally/track/loader.py Outdated Show resolved Hide resolved
esrally/track/loader.py Show resolved Hide resolved
esrally/track/loader.py Outdated Show resolved Hide resolved
esrally/utils/opts.py Outdated Show resolved Hide resolved
esrally/utils/opts.py Outdated Show resolved Hide resolved
integration-test.sh Show resolved Hide resolved
integration-test.sh Show resolved Hide resolved
integration-test.sh Outdated Show resolved Hide resolved
@dliappis
Copy link
Contributor Author

@danielmitterdorfer can you take another pass please? I addressed the refactoring requests and other comments.

@danielmitterdorfer danielmitterdorfer added the :Track Management New operations, changes in the track format, track download changes and the like label May 23, 2019
Copy link
Member

@danielmitterdorfer danielmitterdorfer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left one suggestion to make CompleteTrackParams more robust but apart from that LGTM. Feel free to merge after addressing my comment. Thank you for this massive effort! This is a great usability improvement. :)

esrally/track/loader.py Show resolved Hide resolved
@danielmitterdorfer danielmitterdorfer added the highlight A substantial improvement that is worth mentioning separately in release notes label May 23, 2019
@dliappis dliappis merged commit b93cb00 into elastic:master May 23, 2019
@dliappis dliappis deleted the warn-if-unused-track-param branch May 23, 2019 10:14
@dliappis dliappis modified the milestones: 1.2.0, 1.2.1 Jun 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improves the status quo highlight A substantial improvement that is worth mentioning separately in release notes :Track Management New operations, changes in the track format, track download changes and the like
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can we verify that parameters passed by the user are actually used?
2 participants