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

Allow to benchmark Elasticsearch with and without x-pack #485

Merged
merged 12 commits into from
Apr 27, 2018

Conversation

danielmitterdorfer
Copy link
Member

x-pack, which has been previously implemented as a (closed-source) plugin, has
been recently opened. During that process, x-pack turned from a plugin into a
core module of Elasticsearch. Furthermore, there are now two distributions:

  • The "default" one (with x-pack)
  • An OSS one (without x-pack)

This has several implications on our benchmarks as well. With this commit we
implement the following changes:

  • x-pack is now treated as a "car" in Rally (as it is now a module and not a
    plugin anymore). Strictly speaking, this change has happened in rally-teams
    because Rally does not know about individual plugins.
  • The Gradle build commands are now read from properties in rally-teams
  • Rally can now call bootstrapping code from rally-teams for cars as well
    (previously this was only possible for plugins). One restriction is that we only
    allow to load one bootstrap handler.

@danielmitterdorfer danielmitterdorfer added enhancement Improves the status quo :Benchmark Candidate Management Anything affecting how Rally sets up Elasticsearch labels Apr 26, 2018
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.

Stellar work as usual. LGTM.

try:
return self.car.variables[k]
except KeyError:
raise exceptions.SystemSetupError("Car '{}' misses config variable '{}' to build Elasticsearch.".format(self.car, k))
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe is missing is clearer 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.

Agreed. I changed it.

logger.debug("Component [%s] in config [%s] has no hook registered for phase [%s].",
self.component.name, self.component.config, phase)
Copy link
Contributor

Choose a reason for hiding this comment

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

Newline missing as pointed by GitHub

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

# this should *always* be overridden by the car definition (config base
# variables take least precedence)
heap_size=0
clean_command=./gradlew clean
Copy link
Contributor

Choose a reason for hiding this comment

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

Same newline missing comment

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

@@ -0,0 +1,2 @@
[variables]
verbose_logging=true
Copy link
Contributor

Choose a reason for hiding this comment

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

Same newline missing comment

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

logger.debug("Component [%s] in config [%s] has no hook registered for phase [%s].",
self.component.name, self.component.config, phase)
Copy link
Contributor

Choose a reason for hiding this comment

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

Newline missing here

With this commit we replace all usages of `assert_not_called` with
asserts that assert for a zero call_count. The reason is that
`assert_not_called` has only beeen introduced in Python 3.5 but we also
support (and test) Python 3.4.
@danielmitterdorfer danielmitterdorfer added this to the 0.11.0 milestone Apr 27, 2018
@danielmitterdorfer danielmitterdorfer merged commit 08b36f0 into elastic:master Apr 27, 2018
@danielmitterdorfer danielmitterdorfer deleted the cars-v1 branch May 30, 2018 07:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Benchmark Candidate Management Anything affecting how Rally sets up Elasticsearch enhancement Improves the status quo
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants