Use NavigableSet for representing test version sets, rather than List (#121266)

Author: thecoop

server/src/main/java/org/elasticsearch/ReleaseVersions.java

@@ -78,10 +78,10 @@ public static IntFunction<String> generateVersionsLookup(Class<?> versionContain
             // replace all version lists with the smallest & greatest versions
             versions.replaceAll((k, v) -> {
                 if (v.size() == 1) {
-                    return List.of(v.get(0));
+                    return List.of(v.getFirst());
                 } else {
                     v.sort(Comparator.naturalOrder());
-                    return List.of(v.get(0), v.get(v.size() - 1));
+                    return List.of(v.getFirst(), v.getLast());
                 }
             });
 
@@ -100,14 +100,14 @@ private static IntFunction<String> lookupFunction(NavigableMap<Integer, List<Ver
 
             String lowerBound, upperBound;
             if (versionRange != null) {
-                lowerBound = versionRange.get(0).toString();
-                upperBound = lastItem(versionRange).toString();
+                lowerBound = versionRange.getFirst().toString();
+                upperBound = versionRange.getLast().toString();
             } else {
                 // infer the bounds from the surrounding entries
                 var lowerRange = versions.lowerEntry(id);
                 if (lowerRange != null) {
                     // the next version is just a guess - might be a newer revision, might be a newer minor or major...
-                    lowerBound = nextVersion(lastItem(lowerRange.getValue())).toString();
+                    lowerBound = nextVersion(lowerRange.getValue().getLast()).toString();
                 } else {
                     // a really old version we don't have a record for
                     // assume it's an old version id - we can just return it directly
@@ -122,7 +122,7 @@ private static IntFunction<String> lookupFunction(NavigableMap<Integer, List<Ver
                 var upperRange = versions.higherEntry(id);
                 if (upperRange != null) {
                     // too hard to guess what version this id might be for using the next version - just use it directly
-                    upperBound = upperRange.getValue().get(0).toString();
+                    upperBound = upperRange.getValue().getFirst().toString();
                 } else {
                     // a newer version than all we know about? Can't map it...
                     upperBound = "[" + id + "]";
@@ -133,10 +133,6 @@ private static IntFunction<String> lookupFunction(NavigableMap<Integer, List<Ver
         };
     }
 
-    private static <T> T lastItem(List<T> list) {
-        return list.get(list.size() - 1);
-    }
-
     private static Version nextVersion(Version version) {
         return new Version(version.id + 100);   // +1 to revision
     }

server/src/main/java/org/elasticsearch/index/IndexVersions.java

@@ -24,7 +24,6 @@
 import java.util.TreeMap;
 import java.util.TreeSet;
 import java.util.function.IntFunction;
-import java.util.stream.Collectors;
 
 @SuppressWarnings("deprecation")
 public class IndexVersions {
@@ -250,10 +249,6 @@ static NavigableMap<Integer, IndexVersion> getAllVersionIds(Class<?> cls) {
         return Collections.unmodifiableNavigableMap(builder);
     }
 
-    static Collection<IndexVersion> getAllWriteVersions() {
-        return VERSION_IDS.values().stream().filter(v -> v.onOrAfter(IndexVersions.MINIMUM_COMPATIBLE)).collect(Collectors.toSet());
-    }
-
     static Collection<IndexVersion> getAllVersions() {
         return VERSION_IDS.values();
     }

server/src/main/java/org/elasticsearch/internal/VersionExtension.java

@@ -12,16 +12,16 @@
 import org.elasticsearch.TransportVersion;
 import org.elasticsearch.index.IndexVersion;
 
-import java.util.List;
+import java.util.Collection;
 
 /**
  * Allows plugging in current version elements.
  */
 public interface VersionExtension {
     /**
-     * Returns list of {@link TransportVersion} defined by extension
+     * Returns additional {@link TransportVersion} defined by extension
      */
-    List<TransportVersion> getTransportVersions();
+    Collection<TransportVersion> getTransportVersions();
 
     /**
      * Returns the {@link IndexVersion} that Elasticsearch should use.

server/src/test/java/org/elasticsearch/TransportVersionTests.java

@@ -13,15 +13,13 @@
 import org.elasticsearch.test.TransportVersionUtils;
 
 import java.lang.reflect.Modifier;
-import java.util.Collections;
-import java.util.List;
 import java.util.Set;
 import java.util.regex.Matcher;
 import java.util.regex.Pattern;
 
+import static org.hamcrest.Matchers.contains;
 import static org.hamcrest.Matchers.containsString;
 import static org.hamcrest.Matchers.endsWith;
-import static org.hamcrest.Matchers.equalTo;
 import static org.hamcrest.Matchers.greaterThan;
 import static org.hamcrest.Matchers.is;
 import static org.hamcrest.Matchers.lessThan;
@@ -69,13 +67,11 @@ public static class DuplicatedIdFakeVersion {
     public void testStaticTransportVersionChecks() {
         assertThat(
             TransportVersions.collectAllVersionIdsDefinedInClass(CorrectFakeVersion.class),
-            equalTo(
-                List.of(
-                    CorrectFakeVersion.V_0_000_002,
-                    CorrectFakeVersion.V_0_000_003,
-                    CorrectFakeVersion.V_0_000_004,
-                    CorrectFakeVersion.V_0_00_01
-                )
+            contains(
+                CorrectFakeVersion.V_0_000_002,
+                CorrectFakeVersion.V_0_000_003,
+                CorrectFakeVersion.V_0_000_004,
+                CorrectFakeVersion.V_0_00_01
             )
         );
         AssertionError e = expectThrows(
@@ -184,7 +180,7 @@ public void testVersionConstantPresent() {
     }
 
     public void testCURRENTIsLatest() {
-        assertThat(Collections.max(TransportVersion.getAllVersions()), is(TransportVersion.current()));
+        assertThat(TransportVersion.getAllVersions().getLast(), is(TransportVersion.current()));
     }
 
     public void testPatchVersionsStillAvailable() {

test/framework/src/main/java/org/elasticsearch/index/KnownIndexVersions.java

@@ -9,7 +9,9 @@
 
 package org.elasticsearch.index;
 
-import java.util.List;
+import java.util.Collections;
+import java.util.NavigableSet;
+import java.util.TreeSet;
 
 /**
  * Provides access to all known index versions
@@ -18,10 +20,12 @@ public class KnownIndexVersions {
     /**
      * A sorted list of all known index versions
      */
-    public static final List<IndexVersion> ALL_VERSIONS = List.copyOf(IndexVersions.getAllVersions());
+    public static final NavigableSet<IndexVersion> ALL_VERSIONS = Collections.unmodifiableNavigableSet(
+        new TreeSet<>(IndexVersions.getAllVersions())
+    );
 
     /**
      * A sorted list of all known index versions that can be written to
      */
-    public static final List<IndexVersion> ALL_WRITE_VERSIONS = List.copyOf(IndexVersions.getAllWriteVersions());
+    public static final NavigableSet<IndexVersion> ALL_WRITE_VERSIONS = ALL_VERSIONS.tailSet(IndexVersions.MINIMUM_COMPATIBLE, true);
 }

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

@@ -14,7 +14,7 @@
 import org.elasticsearch.xcontent.ToXContent;
 
 import java.io.IOException;
-import java.util.List;
+import java.util.Collection;
 
 import static org.elasticsearch.test.BWCVersions.DEFAULT_BWC_VERSIONS;
 
@@ -28,7 +28,7 @@ public abstract class AbstractBWCSerializationTestCase<T extends Writeable & ToX
     /**
      * The bwc versions to test serialization against
      */
-    protected List<TransportVersion> bwcVersions() {
+    protected Collection<TransportVersion> bwcVersions() {
         return DEFAULT_BWC_VERSIONS;
     }
 

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

@@ -12,17 +12,14 @@
 import org.elasticsearch.TransportVersion;
 import org.elasticsearch.TransportVersions;
 
-import java.util.Collections;
-import java.util.List;
+import java.util.NavigableSet;
 
 public final class BWCVersions {
     private BWCVersions() {}
 
-    public static List<TransportVersion> getAllBWCVersions() {
-        List<TransportVersion> allVersions = TransportVersion.getAllVersions();
-        int minCompatVersion = Collections.binarySearch(allVersions, TransportVersions.MINIMUM_COMPATIBLE);
-        return allVersions.subList(minCompatVersion, allVersions.size());
+    public static NavigableSet<TransportVersion> getAllBWCVersions() {
+        return TransportVersionUtils.allReleasedVersions().tailSet(TransportVersions.MINIMUM_COMPATIBLE, true);
     }
 
-    public static final List<TransportVersion> DEFAULT_BWC_VERSIONS = getAllBWCVersions();
+    public static final NavigableSet<TransportVersion> DEFAULT_BWC_VERSIONS = getAllBWCVersions();
 }

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

@@ -14,15 +14,23 @@
 import org.elasticsearch.core.Nullable;
 
 import java.util.Collections;
-import java.util.List;
+import java.util.NavigableSet;
 import java.util.Random;
 import java.util.Set;
+import java.util.TreeSet;
 import java.util.stream.Collectors;
 
+import static org.apache.lucene.tests.util.LuceneTestCase.random;
+
 public class TransportVersionUtils {
+
+    private static final NavigableSet<TransportVersion> RELEASED_VERSIONS = Collections.unmodifiableNavigableSet(
+        new TreeSet<>(TransportVersion.getAllVersions())
+    );
+
     /** Returns all released versions */
-    public static List<TransportVersion> allReleasedVersions() {
-        return TransportVersion.getAllVersions();
+    public static NavigableSet<TransportVersion> allReleasedVersions() {
+        return RELEASED_VERSIONS;
     }
 
     /** Returns the oldest known {@link TransportVersion} */
@@ -32,7 +40,7 @@ public static TransportVersion getFirstVersion() {
 
     /** Returns a random {@link TransportVersion} from all available versions. */
     public static TransportVersion randomVersion() {
-        return ESTestCase.randomFrom(allReleasedVersions());
+        return VersionUtils.randomFrom(random(), allReleasedVersions(), TransportVersion::fromId);
     }
 
     /** Returns a random {@link TransportVersion} from all available versions without the ignore set */
@@ -42,7 +50,7 @@ public static TransportVersion randomVersion(Set<TransportVersion> ignore) {
 
     /** Returns a random {@link TransportVersion} from all available versions. */
     public static TransportVersion randomVersion(Random random) {
-        return allReleasedVersions().get(random.nextInt(allReleasedVersions().size()));
+        return VersionUtils.randomFrom(random, allReleasedVersions(), TransportVersion::fromId);
     }
 
     /** Returns a random {@link TransportVersion} between <code>minVersion</code> and <code>maxVersion</code> (inclusive). */
@@ -55,24 +63,21 @@ public static TransportVersion randomVersionBetween(
             throw new IllegalArgumentException("maxVersion [" + maxVersion + "] cannot be less than minVersion [" + minVersion + "]");
         }
 
-        int minVersionIndex = 0;
-        List<TransportVersion> allReleasedVersions = allReleasedVersions();
+        NavigableSet<TransportVersion> versions = allReleasedVersions();
         if (minVersion != null) {
-            minVersionIndex = Collections.binarySearch(allReleasedVersions, minVersion);
+            if (versions.contains(minVersion) == false) {
+                throw new IllegalArgumentException("minVersion [" + minVersion + "] does not exist.");
+            }
+            versions = versions.tailSet(minVersion, true);
         }
-        int maxVersionIndex = allReleasedVersions.size() - 1;
         if (maxVersion != null) {
-            maxVersionIndex = Collections.binarySearch(allReleasedVersions, maxVersion);
-        }
-        if (minVersionIndex < 0) {
-            throw new IllegalArgumentException("minVersion [" + minVersion + "] does not exist.");
-        } else if (maxVersionIndex < 0) {
-            throw new IllegalArgumentException("maxVersion [" + maxVersion + "] does not exist.");
-        } else {
-            // minVersionIndex is inclusive so need to add 1 to this index
-            int range = maxVersionIndex + 1 - minVersionIndex;
-            return allReleasedVersions.get(minVersionIndex + random.nextInt(range));
+            if (versions.contains(maxVersion) == false) {
+                throw new IllegalArgumentException("maxVersion [" + maxVersion + "] does not exist.");
+            }
+            versions = versions.headSet(maxVersion, true);
         }
+
+        return VersionUtils.randomFrom(random, versions, TransportVersion::fromId);
     }
 
     public static TransportVersion getPreviousVersion() {
@@ -82,42 +87,28 @@ public static TransportVersion getPreviousVersion() {
     }
 
     public static TransportVersion getPreviousVersion(TransportVersion version) {
-        int place = Collections.binarySearch(allReleasedVersions(), version);
-        if (place < 0) {
-            // version does not exist - need the item before the index this version should be inserted
-            place = -(place + 1);
-        }
-
-        if (place < 1) {
+        TransportVersion lower = allReleasedVersions().lower(version);
+        if (lower == null) {
             throw new IllegalArgumentException("couldn't find any released versions before [" + version + "]");
         }
-        return allReleasedVersions().get(place - 1);
+        return lower;
     }
 
     public static TransportVersion getNextVersion(TransportVersion version) {
         return getNextVersion(version, false);
     }
 
     public static TransportVersion getNextVersion(TransportVersion version, boolean createIfNecessary) {
-        List<TransportVersion> allReleasedVersions = allReleasedVersions();
-        int place = Collections.binarySearch(allReleasedVersions, version);
-        if (place < 0) {
-            // version does not exist - need the item at the index this version should be inserted
-            place = -(place + 1);
-        } else {
-            // need the *next* version
-            place++;
-        }
-
-        if (place < 0 || place >= allReleasedVersions.size()) {
+        TransportVersion higher = allReleasedVersions().higher(version);
+        if (higher == null) {
             if (createIfNecessary) {
                 // create a new transport version one greater than specified
                 return new TransportVersion(version.id() + 1);
             } else {
                 throw new IllegalArgumentException("couldn't find any released versions after [" + version + "]");
             }
         }
-        return allReleasedVersions.get(place);
+        return higher;
     }
 
     /** Returns a random {@code TransportVersion} that is compatible with {@link TransportVersion#current()} */