Use more specific name for mandatory plugin check selectively #446
Conversation
Commit 7a051f3 reverted the use of a more specific plugin name for the mandatory plugin check (e.g. specifying `mandatory.plugin=x-pack` instead of ``mandatory.plugin=x-pack-security`), however, more specific names are still required for benchmarking release versions <6.3. Selectively use a more specific name for the mandatory plugin check depending on the Elasticsearch version. Also add test cases for both scenarios. Relates: 7a051f3 Relates: elastic/elasticsearch#28710
esrally/mechanic/provisioner.py
Outdated
| (versions.major_version(self.distribution_version) < 6)): | ||
| mandatory_plugins.append(installer.sub_plugin_name) | ||
| else: | ||
| raise NameError |
There was a problem hiding this comment.
I don't understand the need to raise NameError here.
There was a problem hiding this comment.
This is so that that if the version of Elasticsearch is >=6.3, the non-specific plugin name is applied (inside the exception code) i.e. force the execution inside the exception block.
There was a problem hiding this comment.
Hmm, but I mean it's not really an error condition if the version is >= 6.3? We could just set the plugin name with mandatory_plugins.append(installer.plugin_name) instead of raising an error which seems more straightforward to read to me.
esrally/mechanic/provisioner.py
Outdated
| mandatory_plugins.append(installer.plugin_name) | ||
| # For Elasticsearch < 6.3 more specific plugin names are required for mandatory plugin check | ||
| # Details in: https://github.com/elastic/elasticsearch/pull/28710 | ||
| try: |
There was a problem hiding this comment.
I understand that it is a necessity at the moment but I think we should try to be version-agnostic in provisioner code. When earlier 6.x versions are not in use anymore I think we should remove this again to stay version-agnostic.
There was a problem hiding this comment.
I agree with you. I was hesitant to add this conditional here and wasn't 100% sure in which place to define the constraints themselves. Should I add a TODO: to remove this when <6.3 is EOL?
There was a problem hiding this comment.
yes, please add a TODO.
esrally/mechanic/provisioner.py
Outdated
| # For Elasticsearch < 6.3 more specific plugin names are required for mandatory plugin check | ||
| # Details in: https://github.com/elastic/elasticsearch/pull/28710 | ||
| try: | ||
| if ((versions.major_version(self.distribution_version) == 6 and |
There was a problem hiding this comment.
Alternatively you could use major, minor, _, _ = versions.components(self.distribution_version). Then there would also be no need for a new helper function minor_version in the versions module.
There was a problem hiding this comment.
Yes good suggestion! I felt like simplifying the code inside the _provisioner_variables() method with no need to assign temporary variables but there are ways around it and I agree let's not add another helper function.
esrally/mechanic/provisioner.py
Outdated
| mandatory_plugins.append(installer.sub_plugin_name) | ||
| else: | ||
| raise NameError | ||
| except (NameError, TypeError, exceptions.InvalidSyntax): |
There was a problem hiding this comment.
I think a TypeError should not occur at this point? If the individual components of the version string are not int they would not match the regex.
There was a problem hiding this comment.
Yup it was added because the integration tests raise this, if not present:
[ERROR] Cannot race. expected string or bytes-like object
Traceback (most recent call last):
File "/home/dl/source/elastic/dliappis_forks/rally/.tox/py36/lib/python3.6/site-packages/esrally/mechanic/mechanic.py", line 516, in receiveMsg_StartNodes
nodes = self.mechanic.start_engine()
File "/home/dl/source/elastic/dliappis_forks/rally/.tox/py36/lib/python3.6/site-packages/esrally/mechanic/mechanic.py", line 645, in start_engine
node_configs.append(p.prepare(binaries))
File "/home/dl/source/elastic/dliappis_forks/rally/.tox/py36/lib/python3.6/site-packages/esrally/mechanic/provisioner.py", line 164, in prepare
provisioner_vars = self._provisioner_variables()
File "/home/dl/source/elastic/dliappis_forks/rally/.tox/py36/lib/python3.6/site-packages/esrally/mechanic/provisioner.py", line 192, in _provisioner_variables
if ((versions.major_version(self.distribution_version) == 6 and
File "/home/dl/source/elastic/dliappis_forks/rally/.tox/py36/lib/python3.6/site-packages/esrally/utils/versions.py", line 25, in major_version
major, _, _, _ = components(version)
File "/home/dl/source/elastic/dliappis_forks/rally/.tox/py36/lib/python3.6/site-packages/esrally/utils/versions.py", line 49, in components
matches = versions_pattern.match(version)
TypeError: expected string or bytes-like object
|
@danielmitterdorfer I addressed the comments, could you please have a look again? |
danielmitterdorfer
added
the
:Benchmark Candidate Management
Commit 7a051f3 reverted the use of a
more specific plugin name for the mandatory plugin
check (e.g. specifying
mandatory.plugin=x-packinstead of``mandatory.plugin=x-pack-security`), however, more specific names are
still required for benchmarking release versions <6.3.
Selectively use a more specific name for the mandatory plugin check
depending on the Elasticsearch version.
Also add test cases for both scenarios.
Relates: 7a051f3
Relates: elastic/elasticsearch#28710