Fallback to closest minor branch for track/team repositories #1165
Conversation
So far Rally's fallback logic for picking the track/team repository branch has been basically either that of exact match, or failing that use the matching major version. This means that e.g. a major change introduced in 7.11 would mean that e.g. rally-teams would require branches 7.11, 7.12 etc. throughout the entire lifecycle of the 7 version. This commit alters this fallback logic and instead will prefer the nearest minor branch, if available.
dliappis
added
enhancement
docs/track.rst
Outdated
| Rally tries to use the branch with the best match to the benchmarked version of Elasticsearch. | ||
| Assuming the track repository has several branches, the order is: | ||
|
|
||
| 1. Exact branch matches; e.g. if the repo contains branches `7`, `7.1` and `7.10.2` and Elasticsearch version is `7.10.2`, `7.10.2` will be chosen as the track repo. |
There was a problem hiding this comment.
Maybe say "will be chosen as branch" or "checked out" instead of "will be chosen as the track repo"?
There was a problem hiding this comment.
Addressed in 3ae2fa4
docs/track.rst
Outdated
| Assuming the track repository has several branches, the order is: | ||
|
|
||
| 1. Exact branch matches; e.g. if the repo contains branches `7`, `7.1` and `7.10.2` and Elasticsearch version is `7.10.2`, `7.10.2` will be chosen as the track repo. | ||
| 2. Nearest minor matches; e.g. if the repo contains branches `7`, `7.1` and `7.10` and Elasticsearch version is `7.10.2`, `7.10` will be chosen as the track repo. Alternatively if version is `7.9`, `7.1` will be chosen. |
There was a problem hiding this comment.
I think the term "nearest" is too vague. Consider the version is 7.8.0 and we have branches 7.9 and 7.1. To me, 7.9 is "nearer than" 7.1 because it's only one minor version distance. Therefore we need to refer to this behavior more accurately, e.g. as "nearest prior minor"?
Note that the term "nearest" is used elsewhere too (e.g. in code comments).
There was a problem hiding this comment.
Addressed in 861a176
esrally/utils/repo.py
Outdated
| @@ -103,7 +103,7 @@ def update(self, distribution_version): | |||
|
|
|||
| def _find_matching_tag(self, distribution_version): | |||
| tags = git.tags(self.repo_dir) | |||
| for version in versions.versions(distribution_version): | |||
| for version, _ in versions.VersionVariants(distribution_version).all_versions: | |||
There was a problem hiding this comment.
Would it make sense to expose this as a function instead? This would maybe read a bit nicer:
for version, _ in versions.variants_of(distribution_version):
...There was a problem hiding this comment.
The class VersionVariants is used in utils/versions.py leveraging state. repo.py doesn't require state, just the list expansion of all versions.
Are you suggesting variants_of() could be a utility function within util/versions.py that does the object instantiation/calling of .all_versions() or?
There was a problem hiding this comment.
Yes, I am proposing a utility function along the lines of:
def variants_of(distribution_version):
for version, _ in versions.VersionVariants(distribution_version).all_versions
yield versionThis implementation would even get rid of the superfluous second return value of #all_versions() and give us the chance to keep the class VersionVariants internal to to the versions module.
There was a problem hiding this comment.
Ok fixed in 13ba1ad
tests/utils/versions_test.py
Outdated
| "Best match for specific minor version" | ||
|
|
||
| assert versions.best_match(["7", "7.11", "7.2", "5", "6", "master"], "7.12.0") == "7.11",\ | ||
| "If no exact match, best match is the closest minor" |
There was a problem hiding this comment.
I interpret "closest" as a synonym for "nearest" in this context. IMHO we should stick to one term consistently.
There was a problem hiding this comment.
Addressed in 861a176
This commit updates the versioning scheme docs to reflect the changes brought in elastic/rally#1165
This commit updates the versioning scheme docs to reflect the changes brought in elastic/rally#1165
This commit updates the versioning scheme docs to reflect the changes brought in elastic/rally#1165
This commit updates the versioning scheme docs to reflect the changes brought in elastic/rally#1165
This commit updates the versioning scheme docs to reflect the changes brought in elastic/rally#1165
This commit updates the versioning scheme docs to reflect the changes brought in elastic/rally#1165
This commit updates the versioning scheme docs to reflect the changes brought in elastic/rally#1165
This commit updates the versioning scheme docs to reflect the changes brought in elastic/rally#1165
This commit updates the versioning scheme docs to reflect the changes brought in elastic/rally#1165
This commit updates the versioning scheme docs to reflect the changes brought in elastic/rally#1165
This commit updates the versioning scheme docs to reflect the changes brought in elastic/rally#1165
This commit updates the versioning scheme docs to reflect the changes brought in elastic/rally#1165
This commit updates the versioning scheme docs to reflect the changes brought in elastic/rally#1165
This commit updates the versioning scheme docs to reflect the changes brought in elastic/rally#1165
This commit updates the versioning scheme docs to reflect the changes brought in elastic/rally#1165
This commit updates the versioning scheme docs to reflect the changes brought in elastic/rally#1165
|
@danielmitterdorfer it occurs to me that the final step, before merging this is to create the following branches:
WDYT? |
|
As discussed offline this sounds reasonable. |
|
Earlier comment has now been addressed:
|
So far Rally's fallback logic for picking the track/team repository
branch has been basically either that of exact match, or failing that
use the matching major version.
This means that e.g. a major change introduced in 7.11 would mean that
e.g. rally-teams would require branches 7.11, 7.12 etc. throughout the
entire lifecycle of the 7 version.
This commit alters this fallback logic and instead will prefer the
nearest minor branch, if available.
Also convert all
utils/versions.pytests topytest.