Implement VersionCollection in Java #34050

alpar-t · 2018-09-25T14:03:35Z

This is not a straight forward rewrite.
It's in preparation of removing the build and snapshot from Version classes in both core and build-tools. As such, the Gradle project will track qualifiers and snapshots in version names, but Elasticsearch will have no knowledge of these.

We used to mark unreleased versions as snapshots in the version collection to differentiate them,
this is no longer possible once we remove that property.
The new implementation replaces the old Groovy one and brings a few changes:

does not track unreleased versions by name. Unfortunately we can't generate Gradle project names dynamically, so the named projects that used to build these unreleased versions are now just numbered placeholders to which we bind the versions in order at configuration time.
- hopefully this approach make the intent of version collection cleaner, and simplifies the implementation of some methods.
- I found the existing names confusing
- no longer uses snapshot in any of the terminology, I found that confusing in the context of bwc testing as it clashes with the snapshot functionality in Elasticsearch.
relaxes equals constraints on the Version class from the build to disregard qualifier and version
different testing strategy based on what's currently in git. These tests were implemented against the Groovy implementation and then ported to Java to assure correctness. Hopefully will make the tests easier to understand.

but with passing tests

Parse the versions from the Java source files and apply rules to figure out which are the wire and index compatible ones from these. On top of that figure out which are unreleased and the branches these live on so we can build them, without coming up with names for each. Since we can't create the Gradle projects dynamically, use place-holders and bind unreleased versions to them by index. Hopefully this is less confusing than the assigning names to the unreleased versions.

elasticmachine · 2018-09-25T14:03:38Z

Pinging @elastic/es-core-infra

alpar-t · 2018-09-25T14:04:29Z

build.gradle

@@ -103,10 +103,6 @@ subprojects {
 * in a branch if there are only betas and rcs in the branch so we have
 * *something* to test against. */
 VersionCollection versions = new VersionCollection(file('server/src/main/java/org/elasticsearch/Version.java').readLines('UTF-8'))
-if (versions.currentVersion != VersionProperties.elasticsearch) {


The check is moved into version collections.
I tried to move checks and decisions there for better visibility.

alpar-t · 2018-09-25T14:09:01Z

distribution/bwc/build.gradle

+    project("$project.path:build-unreleased-version-$unreleasedVersion.index") {
+        Version bwcVersion = unreleasedVersion.version
+        String bwcBranch = unreleasedVersion.branch
+        apply plugin: 'distribution'


Everything below this line is just indented without any other changes except:

some exceptions include the version number that was built

the indenting output stream indents with the version number, so [bwc] becomes e.x. [6.5.0]

rjernst

Thanks for taking on this long overdue cleanup! The more we can convert our gradle code to java the better.

I don't think we should remove the named versions of unreleased versions. The names have very specific meaning that maps to the rules we have about bwc across versions that we want to test. Regarding the use of "snapshot", there are many things in elasticsearch and other software projects that context is needed to understand the semantics of a term. Changing to "unreleased" is fine, though, but I would keep the flag available on Version.

Finally, since this is a rewrite, I don't think a TreeSet is needed here at all. @hub-cap and I discussed its use long ago. See VersionUtils.resolveReleasedVersions for an alternative way to determine which versions are unreleased based on our cross branch compatibility rules. I always meant to go back and change VersionCollection to use the same algorithm, but it was not a high priority.

hub-cap · 2018-09-26T16:05:55Z

I agree w @rjernst that the code he wrote in VersionUtils is wayyyy better than using the TreeSet. Can we instead port the code to look more like that?

Don't duplicate the semantics of index compatible

alpar-t · 2018-09-27T14:48:37Z

I adopted the general idea of groupingBy, but also kept the SortedSet around because I liked how expressive the .last() method is when used like that. I also kept an implementation that looks for unreleased versions to emphasize that this is what the build is primarily interested in ( so we can build them).

I'll rename the projects back as well.

alpar-t · 2018-09-28T11:35:55Z

I renamed the projects and improved the documentation.
Also moved the versionCheck logic to VersionCollection so I can unit test it, and to keep it all in a single place as much as possible.

alpar-t · 2018-09-28T11:42:12Z

@rjernst @hub-cap can you take another look please ?

hub-cap · 2018-09-28T16:02:05Z

Ill have a look after @rjernst +1s the approach. I will say that the code that he wrote was wayyyyy more readable then the hot mess of TreeSet code I originally wrote. He even advised against it, to my chagrin. Boy was I wrong. So, past me wants to let it go, but present me thinks his List<List<Version>> code is much more readable. but i def wont -1 this if @rjernst is happy.

rjernst · 2018-09-28T16:10:53Z

I think the sorting based approach is much more readable (and less complex) and we should not use a TreeSet.

…ifier-snapshot-build

alpar-t · 2018-10-01T11:17:46Z

@rjernst TreeSet is now removed.

…ifier-snapshot-build

rjernst

Thanks for all the changes so far @atorok! I left a lot of little implementation comments. I also think @hub-cap should look at this, at least for the javadocs in VersionCollection, as I know he spent a lot of time on the previous description, and making sure they make sense for another person would be good.

TESTING.asciidoc

build.gradle

rjernst · 2018-10-29T19:43:53Z

build.gradle

-        ext.projectSubstitutions["org.elasticsearch.distribution.zip:elasticsearch-oss:${snapshot}"] = snapshotProject
-      }
+  // substitute unreleased versions with projects that check out and build locally
+  bwcVersions.forPreviousUnreleased { VersionCollection.UnreleasedVersionDescription unreleasedVersion ->


Why do we need a foreach closure? I'm fine with the name change to "unreleased" instead of "snapshot", but can't we just have bwcVersions.unreleased as a collection that we can iterate here as before?

I wanted to avoid creating a collection and returning it when the calling code is really interested in iteration. It could have to be a collection of UnreleasedVersionDescription built each time, or build once cached. I taught this is simpler and easier to read and fits well with Gradle DSL.

We could simply call it unreleased if you think the extra detail in the name makes it actually more confusing.

I think the collection should be created once in the constructor, so there shouldn't be any concern about returning it.

I feel that having the collections as different views of the same data makes the class harder to understand and in this particular case, it won't make a difference in performance large enough to be worthwhile. Do you see other advantages in doing so besides not having to compute it multiple times ?

rjernst · 2018-10-29T19:51:41Z