Skip to content

Commit

Permalink
[ML] Remove Version from MlConfigVersion completely (elastic#100131)
Browse files Browse the repository at this point in the history 
Removed usage of Version from MlConfigVersion.

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.
  • Loading branch information
@droberts195
droberts195 authored Oct 25, 2023
1 parent b19ae8d commit 27956c0
Show file tree
Hide file tree
Showing 4 changed files with 43 additions and 41 deletions.
11 changes: 11 additions & 0 deletions server/src/main/java/org/elasticsearch/cluster/node/DiscoveryNode.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
import java.util.Map;
import java.util.Objects;
import java.util.Optional;
import java.util.OptionalInt;
import java.util.Set;
import java.util.SortedSet;
import java.util.TreeSet;
Expand Down Expand Up @@ -488,6 +489,16 @@ public Version getVersion() {
return this.versionInfo.nodeVersion();
}

public OptionalInt getPre811VersionId() {
// 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_11_0.id) {
return OptionalInt.empty();
}
return OptionalInt.of(versionId);
}

public IndexVersion getMinIndexVersion() {
return versionInfo.minIndexVersion();
}
Expand Down
32 changes: 13 additions & 19 deletions x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/MlConfigVersion.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
Expand Down Expand Up @@ -308,10 +297,10 @@ public static Tuple<MlConfigVersion, MlConfigVersion> 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());
if (mlConfigVerStr != null) {
return fromString(mlConfigVerStr);
}
return fromString(mlConfigVerStr);
return fromId(node.getPre811VersionId().orElseThrow(() -> new IllegalStateException("getting legacy version id not possible")));
}

// Parse an MlConfigVersion from a string.
Expand All @@ -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
Expand Down
23 changes: 3 additions & 20 deletions x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/MlConfigVersionTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,7 @@ public void testGetMlConfigVersionForNode() {
.version(VersionInformation.inferVersions(Version.fromString("8.7.0")))
.build();
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 {
Expand Down Expand Up @@ -232,19 +232,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<MlConfigVersion> ignore = Set.of(MlConfigVersion.ZERO, MlConfigVersion.CURRENT, MlConfigVersion.FIRST_ML_VERSION);
assertThat(MlConfigVersion.CURRENT, sameInstance(MlConfigVersion.fromId(MlConfigVersion.CURRENT.id())));
Expand Down Expand Up @@ -298,13 +285,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());
}
}
18 changes: 16 additions & 2 deletions ...test/java/org/elasticsearch/xpack/ml/inference/ingest/InferenceProcessorFactoryTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -922,8 +922,22 @@ private static ClusterState builderClusterStateWithModelReferences(MlConfigVersi
Set.of(DiscoveryNodeRole.MASTER_ROLE, DiscoveryNodeRole.ML_ROLE, DiscoveryNodeRole.DATA_ROLE)
)
)
.add(DiscoveryNodeUtils.create("current_node", new TransportAddress(InetAddress.getLoopbackAddress(), 9302)))
.add(DiscoveryNodeUtils.create("_node_id", new TransportAddress(InetAddress.getLoopbackAddress(), 9304)))
.add(
DiscoveryNodeUtils.create(
"current_node",
new TransportAddress(InetAddress.getLoopbackAddress(), 9302),
Map.of(MachineLearning.ML_CONFIG_VERSION_NODE_ATTR, MlConfigVersion.CURRENT.toString()),
Set.of(DiscoveryNodeRole.MASTER_ROLE, DiscoveryNodeRole.ML_ROLE, DiscoveryNodeRole.DATA_ROLE)
)
)
.add(
DiscoveryNodeUtils.create(
"_node_id",
new TransportAddress(InetAddress.getLoopbackAddress(), 9304),
Map.of(MachineLearning.ML_CONFIG_VERSION_NODE_ATTR, MlConfigVersion.CURRENT.toString()),
Set.of(DiscoveryNodeRole.MASTER_ROLE, DiscoveryNodeRole.ML_ROLE, DiscoveryNodeRole.DATA_ROLE)
)
)
.localNodeId("_node_id")
.masterNodeId("_node_id")
)
Expand Down

0 comments on commit 27956c0

Please sign in to comment.