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.

Sign up for GitHub

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

Jump to bottom

Warn when cluster settings cannot be applied #542

Merged
merged 2 commits into from
Jul 27, 2018
Merged

Warn when cluster settings cannot be applied #542

merged 2 commits into from
Jul 27, 2018

Conversation

danielmitterdorfer
Copy link
Member

With this commit we issue a warning on the terminal when a user runs a
track that specifies cluster settings but uses the benchmark-only
pipeline. Previously we issued a warning in the logs but this is not the
first place user is looking (nor should they be required to in this
case).

Closes #541

@danielmitterdorfer
Warn when cluster settings cannot be applied
e6b4ea3 
With this commit we issue a warning on the terminal when a user runs a
track that specifies cluster settings but uses the benchmark-only
pipeline. Previously we issued a warning in the logs but this is not the
first place user is looking (nor should they be required to in this
case).

Closes elastic#541
@danielmitterdorfer danielmitterdorfer added bug Something's wrong :Usability Makes Rally easier to use labels Jul 27, 2018
@danielmitterdorfer danielmitterdorfer added this to the 1.0.1 milestone Jul 27, 2018
@danielmitterdorfer
Copy link
Member Author

Example output:

[WARNING] Ensure that these settings are defined in elasticsearch.yml:

{
  "indices.query.bool.max_clause_count": 50000
}

If they are absent, running this track will fail or lead to unexpected results.

I am not so happy about the fact that we render the settings as JSON (ideal would be YAML) but I figured this compromise is ok. The json module from the core library takes care of properly quoting strings and also handles nested structures and I felt adding a new dependency to format one warning message as YAML is unjustified.

dliappis
dliappis approved these changes Jul 27, 2018
View reviewed changes
Copy link
Contributor

@dliappis dliappis left a comment

Choose a reason for hiding this comment

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

Thanks for the quick action here!
I left a question.

esrally/mechanic/mechanic.py Outdated
@@ -270,6 +269,12 @@ def receiveMsg_StartEngine(self, msg, sender):

if msg.external:
self.logger.info("Cluster will not be provisioned by Rally.")
if msg.cluster_settings:
import json
Copy link
Contributor

Choose a reason for hiding this comment

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

I am curious, do you feel that import json at the top of the file as per PEP 8 is risky for the performance or is there some other reason? I know that there are issues with the log level when the elasticsearch module is imported at top level, but unsure about the reason here.

Copy link
Member Author

Choose a reason for hiding this comment

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

It was more to avoid that usage of the json module does not spread across mechanic because I felt it should not. I changed it now to a top-level import.

@danielmitterdorfer
Copy link
Member Author

Thanks for your quick review!

@danielmitterdorfer danielmitterdorfer merged commit 7387ca2 into elastic:master Jul 27, 2018
@danielmitterdorfer danielmitterdorfer deleted the settings-warning branch July 27, 2018 06:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dliappis dliappis dliappis approved these changes

Assignees
No one assigned
Labels
bug Something's wrong :Usability Makes Rally easier to use
Projects
None yet
Milestone
1.0.1
Development

Successfully merging this pull request may close these issues.
2 participants
@danielmitterdorfer @dliappis