From 27e77918b1e9251d9c060e45a40b77a605a49197 Mon Sep 17 00:00:00 2001 From: David Roberts Date: Mon, 2 Oct 2023 10:53:27 +0100 Subject: [PATCH] [ML] Remove Version from MlConfigVersion completely Proof-of-concept to show how Version can be removed from MlConfigVersion if DiscoveryNode is altered to make available the legacy version ID for 8.10.0 or older nodes. DiscoveryNode will have to read this integer off the wire when serialized from these old nodes, so even if Version is removed from DiscoveryNode it will be able to provide the required value. --- .../cluster/node/DiscoveryNode.java | 10 +++++++ .../xpack/core/ml/MlConfigVersion.java | 28 ++++++++----------- .../xpack/core/ml/MlConfigVersionTests.java | 23 ++------------- 3 files changed, 24 insertions(+), 37 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/cluster/node/DiscoveryNode.java b/server/src/main/java/org/elasticsearch/cluster/node/DiscoveryNode.java index ff5496aaef83e..55239eb45f283 100644 --- a/server/src/main/java/org/elasticsearch/cluster/node/DiscoveryNode.java +++ b/server/src/main/java/org/elasticsearch/cluster/node/DiscoveryNode.java @@ -487,6 +487,16 @@ public Version getVersion() { return this.versionInfo.nodeVersion(); } + public int getLegacyVersionIdOrThrow() { + // Even if Version is removed from this class completely it will need to read the version ID + // off the wire for old node versions, so the value of this variable can be obtained from that + int versionId = versionInfo.nodeVersion().id; + if (versionId > Version.V_8_10_0.id) { + throw new IllegalStateException("getting legacy version id [" + versionId + "] not supported"); + } + return versionId; + } + public IndexVersion getMinIndexVersion() { return versionInfo.minIndexVersion(); } diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/MlConfigVersion.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/MlConfigVersion.java index e631e3efe5cb6..c51e418e069d4 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/MlConfigVersion.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/MlConfigVersion.java @@ -268,17 +268,6 @@ public static MlConfigVersion max(MlConfigVersion version1, MlConfigVersion vers return version1.id > version2.id ? version1 : version2; } - // Visible only for testing - static MlConfigVersion fromVersion(Version version) { - if (version.equals(Version.V_8_10_0)) { - return V_10; - } - if (version.after(Version.V_8_10_0)) { - throw new IllegalArgumentException("Cannot convert " + version + ". Incompatible version"); - } - return fromId(version.id); - } - public static MlConfigVersion getMinMlConfigVersion(DiscoveryNodes nodes) { return getMinMaxMlConfigVersion(nodes).v1(); } @@ -309,7 +298,7 @@ public static Tuple getMinMaxMlConfigVersion(D public static MlConfigVersion getMlConfigVersionForNode(DiscoveryNode node) { String mlConfigVerStr = node.getAttributes().get(ML_CONFIG_VERSION_NODE_ATTR); if (mlConfigVerStr == null) { - return fromVersion(node.getVersion()); + return fromId(node.getLegacyVersionIdOrThrow()); } return fromString(mlConfigVerStr); } @@ -329,12 +318,17 @@ public static MlConfigVersion fromString(String str) { if (str.startsWith("8.10.") || str.equals("8.11.0")) { return V_10; } - Matcher matcher = Pattern.compile("^(\\d+)\\.0\\.0$").matcher(str); - int versionNum; - if (matcher.matches() == false || (versionNum = Integer.parseInt(matcher.group(1))) < 10) { - return fromVersion(Version.fromString(str)); + Matcher matcher = Pattern.compile("^(\\d+)\\.(\\d+)\\.(\\d+)(?:-\\w+)?$").matcher(str); + if (matcher.matches() == false) { + throw new IllegalArgumentException("ML config version [" + str + "] not valid"); + } + int first = Integer.parseInt(matcher.group(1)); + int second = Integer.parseInt(matcher.group(2)); + int third = Integer.parseInt(matcher.group(3)); + if (first >= 10 && (second > 0 || third > 0)) { + throw new IllegalArgumentException("ML config version [" + str + "] not valid"); } - return fromId(1000000 * versionNum + 99); + return fromId(1000000 * first + 10000 * second + 100 * third + 99); } @Override diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/MlConfigVersionTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/MlConfigVersionTests.java index 6ead94bbc1fdb..3cc99add7ae32 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/MlConfigVersionTests.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/MlConfigVersionTests.java @@ -172,7 +172,7 @@ public void testGetMlConfigVersionForNode() { VersionInformation.inferVersions(Version.fromString("8.7.0")) ); MlConfigVersion mlConfigVersion1 = MlConfigVersion.getMlConfigVersionForNode(node1); - assertEquals(MlConfigVersion.fromVersion(Version.V_8_5_0), mlConfigVersion1); + assertEquals(MlConfigVersion.V_8_5_0, mlConfigVersion1); } public void testDefinedConstants() throws IllegalAccessException { @@ -237,19 +237,6 @@ public void testMax() { ); } - public void testFromVersion() { - Version version_V_7_7_0 = Version.V_7_0_0; - MlConfigVersion mlConfigVersion_V_7_7_0 = MlConfigVersion.fromVersion(version_V_7_7_0); - assertEquals(version_V_7_7_0.id, mlConfigVersion_V_7_7_0.id()); - - // Version 8.10.0 is treated as if it is MlConfigVersion V_10. - assertEquals(MlConfigVersion.V_10.id(), MlConfigVersion.fromVersion(Version.V_8_10_0).id()); - - // There's no mapping between Version and MlConfigVersion values after Version.V_8_10_0. - Exception e = expectThrows(IllegalArgumentException.class, () -> MlConfigVersion.fromVersion(Version.fromId(8_11_00_99))); - assertEquals("Cannot convert " + Version.fromId(8_11_00_99) + ". Incompatible version", e.getMessage()); - } - public void testVersionConstantPresent() { Set ignore = Set.of(MlConfigVersion.ZERO, MlConfigVersion.CURRENT, MlConfigVersion.FIRST_ML_VERSION); assertThat(MlConfigVersion.CURRENT, sameInstance(MlConfigVersion.fromId(MlConfigVersion.CURRENT.id()))); @@ -303,13 +290,9 @@ public void testFromString() { assertEquals(false, KnownMlConfigVersions.ALL_VERSIONS.contains(unknownVersion)); assertEquals(MlConfigVersion.CURRENT.id() + 1, unknownVersion.id()); - for (String version : new String[] { "10.2", "7.17.2.99" }) { + for (String version : new String[] { "10.2", "7.17.2.99", "9" }) { Exception e = expectThrows(IllegalArgumentException.class, () -> MlConfigVersion.fromString(version)); - assertEquals("the version needs to contain major, minor, and revision, and optionally the build: " + version, e.getMessage()); + assertEquals("ML config version ["+ version + "] not valid", e.getMessage()); } - - String version = "9"; - Exception e = expectThrows(IllegalArgumentException.class, () -> MlConfigVersion.fromString(version)); - assertEquals("the version needs to contain major, minor, and revision, and optionally the build: " + version, e.getMessage()); } }