Skip to content

Commit

Permalink
[ML] Remove Version from MlConfigVersion completely
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
droberts195 committed Oct 2, 2023
1 parent f4a26f3 commit 27e7791
Show file tree
Hide file tree
Showing 3 changed files with 24 additions and 37 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
Expand Down
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 @@ -309,7 +298,7 @@ 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());
return fromId(node.getLegacyVersionIdOrThrow());
}
return fromString(mlConfigVerStr);
}
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
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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<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 @@ -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());
}
}

0 comments on commit 27e7791

Please sign in to comment.