Remove the need for _UNRELEASED suffix in versions #24798

nik9000 · 2017-05-19T14:05:23Z

Before this change we used the name of the constant to check if a version was released or not. Versions ending in _UNRELEASED were considered unreleased and other versions were considered released. The trouble with this is that we'd often forget to switch a version from unreleased to released. That is a problem because unreleased versions don't get backwards compatibility tests. This changes the logic so that the released versions are inferred from the current version using the following rules:

If the current version is a bug fix release all other versions are released.
Otherwise, the version constant before the current version and before all the alphas is considered unreleased.
If that unreleased version is itself a bug fix release then that is the only unreleased version.
Otherwise, the version before it is unreleased.

Some examples:

Right now in master the current version is 6.0.0-alpha2 which means that the last non-alpha in the list is unreleased. That version is 5.5.0. Because that is a .0 release the version before that is also considered unreleased. So in master 5.5.0 and 5.4.1 are both considered unreleased.
Right now in 5.x the current version is 5.5.0 which means that that last non-alpha in the list is unreleased. That version is 5.4.1. It isn't a .0 release, so the only unreleased version is 5.4.1.
Right now in 5.4 the current version is 5.4.1. Since that isn't a .0 release there are no unreleased versions.

This logic is mirrored in both VersionUtils.java and in build.gradle so that gradle can run the appropriate backwards compatibility tests, some of which are already written.

A secondary goal with removing the _UNRELEASED suffix is to one day be able to use a script to add the new version constant.

It is also important to note that gradle already has a test that verifies that the list of released versions is correct according to maven central. This means that we won't be able to forget to add a version constant.

Closes #24768

Attempts to detect if we're missing any `_UNRELEASED` versions from `Version.java`. This is important because this is how we trigger backwards compatibility testing for the HEAD of that branch. If we're building a release with a version number like `x.y.0` then it makes sure that the last version constant is `_UNRELEASED`. Because we validate that the constants are sorted, should work. If that version constant is of the for `x.y.0` then it also checks that there is an `_UNRELEASED` constant before that. Concretely, on master this fails the build unless there is a constant for 5.5.0 and 5.4.1 or just 5.4.1. On 5.x this requires a constant for 5.4.1 only. On 5.4 this doesn't require a constant at all. TODO entirely rework this comment Can't do that Be careful with alphas Oh boy

nik9000 · 2017-05-19T14:06:02Z

core/build.gradle

        .collect { it.toString() })

+    // TODO this is almost certainly going to fail on 5.4 when we release 5.5.0


I want to deal with this in a followup. This change is big enough as is.

What do you mean by "fail on 5.4"? The 5.4 branch? I think we need to filter the actualVersions for things less than the current version?

I believe so. I added this here because I noticed it here. I think it'd be nice to fix it in a separate PR though because it isn't really related to this.

nik9000 · 2017-05-19T14:06:51Z

core/src/main/java/org/elasticsearch/Version.java

@@ -74,15 +74,17 @@
    public static final Version V_5_3_2 = new Version(V_5_3_2_ID, org.apache.lucene.util.Version.LUCENE_6_4_2);
    public static final int V_5_4_0_ID = 5040099;
    public static final Version V_5_4_0 = new Version(V_5_4_0_ID, org.apache.lucene.util.Version.LUCENE_6_5_0);
-    public static final int V_5_5_0_ID_UNRELEASED = 5050099;


I almost certainly should have waited to make this change as a purely mechanical followup. It nasty to backport. But I already bit this pill so I feel like I have to swallow it now....

nik9000 · 2017-05-19T14:07:13Z

core/src/main/java/org/elasticsearch/action/admin/indices/mapping/put/PutMappingRequest.java

@@ -308,7 +308,7 @@ public void readFrom(StreamInput in) throws IOException {
        indicesOptions = IndicesOptions.readIndicesOptions(in);
        type = in.readOptionalString();
        source = in.readString();
-        if (in.getVersion().before(Version.V_6_0_0_alpha1_UNRELEASED)) { // TODO change to V_5_3 once backported
+        if (in.getVersion().before(Version.V_6_0_0_alpha1)) { // TODO change to V_5_3 once backported


This TODO worries me some....

@jaymode, should this change?

Yes it should change. I'll open a PR.

Opened #24835 for both

nik9000 · 2017-05-19T14:09:06Z

...c/main/java/org/elasticsearch/action/admin/indices/template/put/PutIndexTemplateRequest.java

@@ -487,7 +487,7 @@ public void readFrom(StreamInput in) throws IOException {
        for (int i = 0; i < size; i++) {
            final String type = in.readString();
            String mappingSource = in.readString();
-            if (in.getVersion().before(Version.V_6_0_0_alpha1_UNRELEASED)) { // TODO change to V_5_3_0 once backported
+            if (in.getVersion().before(Version.V_6_0_0_alpha1)) { // TODO change to V_5_3_0 once backported


@jaymode here too?

And maybe we're missing something in a mixed cluster test? Maybe we just aren't running it yet.....

This should also change. We do not catch this in our BWC/mixed cluster tests since these checks just guard the need to do content type auto-detection. In versions > 5.3.0 we always send JSON so we do not need to autodetect, but this check being wrong kept the auto detection in place when it wasn't necessary.

nik9000 · 2017-05-19T14:11:56Z

test/framework/src/main/java/org/elasticsearch/test/VersionUtils.java


 /** Utilities for selecting versions in tests */
 public class VersionUtils {
+    /**
+     * Matches names of versions that we process. Intentionally skips things like CURRENT.


Now that I think about it, maybe I should just skip CURRENT and drop the regex entirely.

nik9000 · 2017-05-19T14:13:51Z

test/framework/src/test/java/org/elasticsearch/test/VersionUtilsTests.java

+        }
+    }
+
+    public void testRandomVersionBetween() {


I just moved this test and the one above it. They were in the org.elasticsearch.test.test package which would have forced me to make VersionUtils. resolveReleasedVersions public and that seemed wrong.

nik9000 · 2017-05-19T14:14:29Z

test/framework/src/test/java/org/elasticsearch/test/VersionUtilsTests.java

+        assertEquals(got, Version.CURRENT);
+    }
+
+    static class TestPatchBranch {


I really wanted to have tests of this logic on the types of versions that we expect to see. This is kind of like the current state of the 5.4 branch.

nik9000 · 2017-05-19T14:14:38Z

test/framework/src/test/java/org/elasticsearch/test/VersionUtilsTests.java

+        assertEquals(emptyList(), unreleased);
+    }
+
+    static class TestMinorBranch {


And this one is kind of like 5.x.

nik9000 · 2017-05-19T14:15:47Z

test/framework/src/test/java/org/elasticsearch/test/VersionUtilsTests.java

+        assertEquals(singletonList(TestMinorBranch.V_5_3_2), unreleased);
+    }
+
+    static class TestNextBranch {


This one is what I imagine 5.x looked like right after we forked the 5.4 branch.

Can you rename this TestStableBranch?

nik9000 · 2017-05-19T14:15:56Z

test/framework/src/test/java/org/elasticsearch/test/VersionUtilsTests.java

+    }
+
+    static class TestAlphaBranch {
+        public static final Version V_5_3_0 = Version.fromId(Version.V_5_3_0_ID);


And this one is pretty much like master.

rjernst

I left some comments.

rjernst · 2017-05-19T18:08:07Z

core/build.gradle

        .collect { it.toString() })

+    // TODO this is almost certainly going to fail on 5.4 when we release 5.5.0


What do you mean by "fail on 5.4"? The 5.4 branch? I think we need to filter the actualVersions for things less than the current version?

rjernst · 2017-05-19T18:11:38Z

test/framework/src/main/java/org/elasticsearch/test/VersionUtils.java

+     * Sort versions that have backwards compatibility guarantees from
+     * those that don't. Doesn't actually check whether or not the versions
+     * are released, instead it relies on gradle to have already checked
+     * this which is does in {@code :core:verifyVersions}. So long as the


typo: is -> it

rjernst · 2017-05-19T18:14:35Z

core/build.gradle

@@ -290,7 +290,7 @@ task('verifyVersions') {
    }
    Set<String> knownVersions = new TreeSet<>(xml.versioning.versions.version.collect { it.text() }.findAll { it ==~ /\d\.\d\.\d/ })

-    // Limit the known versions to those that should be wire compatible
+    // Limit the known versions to those that should be index compatible
    String currentVersion = versions.elasticsearch.minus('-SNAPSHOT')
    int prevMajor = Integer.parseInt(currentVersion.split('\\.')[0]) - 1
    if (prevMajor == 4) {


This logic only needs to exist on the 5.x branches.

Yes. I figure it helps to keep them in sync so backporting is easier.

rjernst · 2017-05-19T18:15:27Z

core/build.gradle

@@ -290,7 +290,7 @@ task('verifyVersions') {
    }
    Set<String> knownVersions = new TreeSet<>(xml.versioning.versions.version.collect { it.text() }.findAll { it ==~ /\d\.\d\.\d/ })


It is weird to me this task is on core. Could it just be in the root project? And create a precommit task there which depends on this?

It'd be easy enough to just move the whole thing into the root project. Is that what you'd prefer?

Rereading, yes, that looks like what you'd prefer. I'll do that now.

rjernst · 2017-05-19T18:19:55Z

test/framework/src/main/java/org/elasticsearch/test/VersionUtils.java

+             * If there are, gradle will yell about it. */
+            return new Tuple<>(unmodifiableList(versions), emptyList());
+        }
+        /* If we are on a patch release then we know that at least the version before the


patch release -> stable branch?

Sure. That is more descriptive.

rjernst · 2017-05-19T18:31:57Z

test/framework/src/test/java/org/elasticsearch/test/VersionUtilsTests.java

+    }
+
+    static class TestPatchBranch {
+        public static final Version V_5_3_0 = Version.fromId(Version.V_5_3_0_ID);


Using constants from Version makes removing old constants time consuming. Can you use Version.fromString throughout these tests?

Yeah, that is better.

rjernst · 2017-05-19T18:33:17Z

test/framework/src/test/java/org/elasticsearch/test/VersionUtilsTests.java

+        assertEquals(singletonList(TestMinorBranch.V_5_3_2), unreleased);
+    }
+
+    static class TestNextBranch {


Can you rename this TestStableBranch?

rjernst · 2017-05-19T18:33:40Z

test/framework/src/test/java/org/elasticsearch/test/VersionUtilsTests.java

+        assertEquals(got, Version.CURRENT);
+    }
+
+    static class TestPatchBranch {


Can you rename this TestReleaseBranch?

rjernst · 2017-05-19T18:33:57Z

test/framework/src/test/java/org/elasticsearch/test/VersionUtilsTests.java

+        assertEquals(Arrays.asList(TestNextBranch.V_5_3_2, Version.V_5_4_0), unreleased);
+    }
+
+    static class TestAlphaBranch {


Can you rename this TestUnstableBranch?

rjernst · 2017-05-19T18:35:40Z

test/framework/src/test/java/org/elasticsearch/test/VersionUtilsTests.java

+     * constant and it should be upgraded by hand. Take this opportunity to
+     * make sure that it lines up with reality.
+     */
+    public void testUnreleaseCurrent() {


I don't think we should have a test that fails every time we create a new version; that makes automating version bumping more difficult. Can we instead test that CURRENT is equal to the last unreleased version?

I'll drop it. I had thought it'd be a nice reminder for the next few releases until we automate the version bumping.

…ction for 5.3.0+ This change cleans up some missed TODOs for content type detection on the source of put mapping and put index template requests. In 5.3.0 and newer versions, the source is always JSON so the content type detection is not needed. The TODOs were missed after the change was backported to master. Relates elastic#24798

nik9000 · 2017-05-23T17:28:24Z

@rjernst I think this should be ready for another round.

Adds the `:distribution:bwc-stable` project which builds the previous stable branch when `:distribution:bwc` is building an unreleased branch. When `:distribution:bwc` builds a released branch then `:distribution:bwc-stable` is an empty build, not used or depended on by anything. Relates to elastic#24798

rjernst · 2017-05-25T08:37:08Z

The changes look fine, but I still see the old test class names? I think we should keep with consistent naming, meaning using unstable for master, stable for N.x, and release (no d on the end like I see in one of the class names) for N.M. Albeit I am copying this naming from Lucene, but I have personally used this terminology to talk about ES branching to other engineers at elastic since I've been here, so I think we standardize on it if there is any actual question about it?

…ction for 5.3.0+ (#24835) This change cleans up some missed TODOs for content type detection on the source of put mapping and put index template requests. In 5.3.0 and newer versions, the source is always JSON so the content type detection is not needed. The TODOs were missed after the change was backported to 5.3. Relates #24798

nik9000 · 2017-05-26T15:23:38Z

Ok. I understand the naming convention now. I like it. I'll switch.

Lucene has a standard for branch names and it makes sense, we may as well use that standard when talking about our own branches.

nik9000 · 2017-05-26T15:27:20Z

@rjernst renamed.

rjernst

LGTM, thanks for the naming! I left one last minor comment.

rjernst · 2017-05-26T16:50:47Z

test/framework/src/test/java/org/elasticsearch/test/VersionUtilsTests.java

+        assertEquals(Arrays.asList(TestUnstableBranch.V_5_3_2, Version.V_5_4_0), unreleased);
+    }
+
+    static class TestUnstableBranchWithAlphas {


I don't think this needs "WithAlphas", because the unstable branch always has alphas (when we switch to beta, we cut a new stable branch).

Do you think I should drop the test without alphas then? Or rename it? I'd kind of like to keep it because it is telling is only one of the two tests fails.

We should only have one unstable test, and that should be how the unstable branch Version class always looks, ie with CURRENT being an alpha. I'm not sure I understand the testing concern, you have tests here for each of the patterns we handle on each branch, so we should be covered?

* master: (38 commits) Fix Lucene version expectation Verify Lucene version constants Avoid double decrement on current query counter Remove the need for _UNRELEASED suffix in versions (elastic#24798) Adjust available and free bytes to be non-negative on huge FSes Begin replacing static index tests with full restart tests (elastic#24846) Fix plugin docs for using custom config dir Update context-suggest.asciidoc Move BWC version to 5.5 after backport Support Multiple Collapse Inner Hits Scripting: Rename CompiledType to FactoryType in ScriptContext (elastic#24897) Scripting: Make contexts available to ScriptEngine construction (elastic#24896) Mute index and relocate concurrently Build: Add back explicit exclusions and remove gradle exclusions (elastic#24879) Scripting: Move context definitions to instance type classes (elastic#24883) Build: Fix hadoop integ test error on windows (elastic#24885) Put mapping and index template requests do not need content type detection for 5.3.0+ (elastic#24835) Add the ability to store objects with a ScrollContext (elastic#24777) add docs example for Ingest scripts manipulating document metadata (elastic#24875) Fix error message if an incompatible node connects (elastic#24884) ...

Both gradle and java code attempt to infer the type of a each Version constant in Version.java. It is super important that they infer that each constant has the same type. If they disagree we might accidentally not be testing backwards compatibility for some version. This adds a test to make sure that they agree, modulo known and accepted differences (mostly around alphas). It also changes the minimum wire compatible version from the released 5.4.0 to the unreleased 5.5.0 as that lines up with the gradle logic. Relates to elastic#24798

Both gradle and java code attempt to infer the type of a each Version constant in Version.java. It is super important that they infer that each constant has the same type. If they disagree we might accidentally not be testing backwards compatibility for some version. This adds a test to make sure that they agree, modulo known and accepted differences (mostly around alphas). It also changes the minimum wire compatible version from the released 5.4.0 to the unreleased 5.5.0 as that lines up with the gradle logic. Relates to #24798 Note that the gradle and java version logic doesn't actually match so this contains a hack to make it *look* like it matches. Since this is a start, I'm merging it and going to work on some followups to make the logic actually match.....

nik9000 added 5 commits May 19, 2017 09:37

Merge branch 'master' into version_check_some_more

e9c0251

Add reality check

cfa8154

Explain

3b9d569

TODO

405deef

nik9000 added :Internal >enhancement v5.5.0 v6.0.0 WIP labels May 19, 2017

nik9000 requested review from rjernst and jasontedor May 19, 2017 14:05

Formatting

fa1a4e0

nik9000 commented May 19, 2017

View reviewed changes

nik9000 added review and removed WIP labels May 19, 2017

nik9000 mentioned this pull request May 19, 2017

Ensure Version#CURRENT is not _UNRELEASED if we are on a release branch #24768

Closed

rjernst reviewed May 19, 2017

View reviewed changes

rjernst mentioned this pull request May 22, 2017

Removes the 5.3.3 version constant #24833

Merged

jaymode mentioned this pull request May 23, 2017

Put mapping and index template requests do not need content type detection for 5.3.0+ #24835

Merged

nik9000 added 6 commits May 23, 2017 12:51

Mostly renames

a2376ed

Merge branch 'master' into version_check_some_more

43a11b3

Merge branch 'master' into version_check_some_more

1b4223b

More renames

ab38eb5

Pitch the regex

8db2ced

Move verifyVersions

581abea

nik9000 mentioned this pull request May 24, 2017

Rework bwc snapshot projects to build up to two bwc versions #24870

Merged

Rename to match Lucene's branch names

351a970

Lucene has a standard for branch names and it makes sense, we may as well use that standard when talking about our own branches.

rjernst approved these changes May 26, 2017

View reviewed changes

nik9000 added 4 commits May 26, 2017 14:16

Merge branch 'master' into version_check_some_more

a9a9d03

Merge branch 'master' into version_check_some_more

9989cd3

Raname sneaky version

089d2b2

Rename

bb62229

nik9000 removed the v5.5.0 label May 26, 2017

nik9000 merged commit 5da8ce8 into elastic:master May 26, 2017

nik9000 mentioned this pull request May 29, 2017

Test that gradle and Java version types match #24943

Merged

nik9000 deleted the version_check_some_more branch June 7, 2017 14:51

clintongormley added v6.0.0-beta1 and removed v6.0.0 labels Jul 25, 2017

		.collect { it.toString() })

		// TODO this is almost certainly going to fail on 5.4 when we release 5.5.0

		@@ -290,7 +290,7 @@ task('verifyVersions') {
		}
		Set<String> knownVersions = new TreeSet<>(xml.versioning.versions.version.collect { it.text() }.findAll { it ==~ /\d\.\d\.\d/ })

Remove the need for _UNRELEASED suffix in versions #24798

Remove the need for _UNRELEASED suffix in versions #24798

Conversation

nik9000 commented May 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjernst left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nik9000 commented May 23, 2017

rjernst commented May 25, 2017

nik9000 commented May 26, 2017

nik9000 commented May 26, 2017

rjernst left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nik9000 commented May 19, 2017 •

edited

Loading