Derive JDK version at runtime #518
Conversation
With this commit we derive the appropriate JDK version at benchmark runtime instead of requiring that it is preconfigured. This requires two new variables in the rally-teams repo: `build.jdk` and `runtime.jdk` to resolve the appropriate (major) JDK version depending on the Elasticsearch version. When we know the required JDK version, we check the environment variables `JAVAx_HOME` (where `x` is the JDK version) and `JAVA_HOME` to find the correct JAVA_HOME path. We also allow the user to override the runtime JDK with a new command line parameter `--runtime-jdk` as Rally will default to the highest available and supported JDK version on the target system. Closes #452
danielmitterdorfer
added
enhancement
docs/quickstart.rst
Outdated
| @@ -4,7 +4,7 @@ Quickstart | |||
| Install | |||
| ------- | |||
|
|
|||
| Install Python 3.4+ including ``pip3``, git 1.9+ and at least JDK 8. If you want to benchmark source builds of Elasticsearch you also need JDK 10. Then run the following command, optionally prefixed by ``sudo`` if necessary:: | |||
| Install Python 3.4+ including ``pip3``, git 1.9+ and an appropriate JDK to run Elasticsearch Be sure that ``JAVA_HOME`` points to that JDK. Then run the following command, optionally prefixed by ``sudo`` if necessary:: | |||
There was a problem hiding this comment.
Should we link the JVM support matrix here? https://www.elastic.co/support/matrix#matrix_jvm
esrally/mechanic/team.py
Outdated
| try: | ||
| return self.variables[name] | ||
| except KeyError: | ||
| raise exceptions.SystemSetupError("Car \"{}\" misses mandatory config key \"{}\"".format(self.name, name)) |
There was a problem hiding this comment.
Nit/English comment: "is missing". Misses here implies an emotional state i.e. wishes it would be present.
There was a problem hiding this comment.
I rewrote it to: "Car ... requires config key ..."
esrally/utils/jvm.py
Outdated
| def resolve_path(majors, sysprop_reader=system_property): | ||
| """ | ||
| Resolves the path to the JDK with the provided major version(s). It checks the provided versions in order and will return the first |
There was a problem hiding this comment.
in order --> are in order
There was a problem hiding this comment.
I meant something different here, namely that the resolution depends on the order in the list provided by the parameter majors. So it will behave differently when you pass [7, 8, 9] instead of [9, 8, 7].
I rewrote it to:
It checks the versions in the same order specified in ``majors`` and will return the first match.
esrally/mechanic/launcher.py
Outdated
| env = {} | ||
| env.update(os.environ) | ||
| env.update(car.env) | ||
| # Unix specific!: | ||
| self._set_env(env, "PATH", "%s/bin" % self.java_home, separator=":") | ||
| self._set_env(env, "PATH", "%s/bin" % java_home, separator=":") |
There was a problem hiding this comment.
Nit: I don't think we are in the hot code path here, could be replaced with {}.format
There was a problem hiding this comment.
Should we use the more OS agnostic os.pathsep for the separator here (and maybe even as the default in _set_env?
There was a problem hiding this comment.
Good catch(es)! I also used os.path.join(java_home, "bin") now instead of string formatting.
esrally/utils/jvm.py
Outdated
| """ | ||
| Resolves the path to the JDK with the provided major version(s). It checks the provided versions in order and will return the first | ||
| match. For that it checks first the environment variable ``JAVAx_HOME`` where ``x`` is the checked major version and will fall back |
There was a problem hiding this comment.
docstring: Could be re-written as:
To achieve this, it first checks the major version
xin the environment variableJAVAx_HOMEand falls back toJAVA_HOME.
Align Dockerfile with JAVAx_HOME environment variable requirements introduced in: elastic#518
Align Dockerfile with JAVAx_HOME environment variable requirements introduced in #518
With this commit we derive the appropriate JDK version at benchmark
runtime instead of requiring that it is preconfigured. This requires two
new variables in the rally-teams repo:
build.jdkandruntime.jdktoresolve the appropriate (major) JDK version depending on the
Elasticsearch version.
When we know the required JDK version, we check the environment
variables
JAVAx_HOME(wherexis the JDK version) andJAVA_HOMEtofind the correct JAVA_HOME path. We also allow the user to override the
runtime JDK with a new command line parameter
--runtime-jdkas Rallywill default to the highest available and supported JDK version on the
target system.
Closes #452