Skip to content

Commit d9f6451

Browse files
thecoopthecoop
authored
Remove BuildVersion.id() (#115795)
BuildVersion should be entirely opaque, it should be only possible to do equality comparisons
1 parent c77fb33 commit d9f6451

File tree

8 files changed

+85
-17
lines changed

8 files changed

+85
-17
lines changed

‎server/src/main/java/org/elasticsearch/env/BuildVersion.java‎

Lines changed: 35 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -11,9 +11,13 @@
1111

1212
import org.elasticsearch.Build;
1313
import org.elasticsearch.Version;
14+
import org.elasticsearch.common.io.stream.StreamInput;
15+
import org.elasticsearch.common.io.stream.Writeable;
1416
import org.elasticsearch.internal.BuildExtension;
1517
import org.elasticsearch.plugins.ExtensionLoader;
18+
import org.elasticsearch.xcontent.ToXContentFragment;
1619

20+
import java.io.IOException;
1721
import java.util.ServiceLoader;
1822

1923
/**
@@ -31,7 +35,7 @@
3135
* provide versions that accommodate different release models or versioning
3236
* schemes.</p>
3337
*/
34-
public abstract class BuildVersion {
38+
public abstract class BuildVersion implements ToXContentFragment, Writeable {
3539

3640
/**
3741
* Check whether this version is on or after a minimum threshold.
@@ -58,6 +62,11 @@ public abstract class BuildVersion {
5862
*/
5963
public abstract boolean isFutureVersion();
6064

65+
/**
66+
* Returns this build version in a form suitable for storing in node metadata
67+
*/
68+
public abstract String toNodeMetadata();
69+
6170
/**
6271
* Create a {@link BuildVersion} from a version ID number.
6372
*
@@ -72,6 +81,16 @@ public static BuildVersion fromVersionId(int versionId) {
7281
return CurrentExtensionHolder.BUILD_EXTENSION.fromVersionId(versionId);
7382
}
7483

84+
/**
85+
* Create a {@link BuildVersion} from a version in node metadata
86+
*
87+
* @param version The string stored in node metadata
88+
* @return a version representing a build or release of Elasticsearch
89+
*/
90+
public static BuildVersion fromNodeMetadata(String version) {
91+
return CurrentExtensionHolder.BUILD_EXTENSION.fromNodeMetadata(version);
92+
}
93+
7594
/**
7695
* Create a {@link BuildVersion} from a version string.
7796
*
@@ -82,6 +101,16 @@ public static BuildVersion fromString(String version) {
82101
return CurrentExtensionHolder.BUILD_EXTENSION.fromString(version);
83102
}
84103

104+
/**
105+
* Read a {@link BuildVersion} from an input stream
106+
*
107+
* @param input The stream to read
108+
* @return a version representing a build or release of Elasticsearch
109+
*/
110+
public static BuildVersion fromStream(StreamInput input) throws IOException {
111+
return CurrentExtensionHolder.BUILD_EXTENSION.fromStream(input);
112+
}
113+
85114
/**
86115
* Get the current build version.
87116
*
@@ -94,9 +123,6 @@ public static BuildVersion current() {
94123
return CurrentExtensionHolder.BUILD_EXTENSION.currentBuildVersion();
95124
}
96125

97-
// only exists for NodeMetadata#toXContent
98-
public abstract int id();
99-
100126
private static class CurrentExtensionHolder {
101127
private static final BuildExtension BUILD_EXTENSION = findExtension();
102128

@@ -125,6 +151,10 @@ public BuildVersion fromVersionId(int versionId) {
125151
public BuildVersion fromString(String version) {
126152
return new DefaultBuildVersion(version);
127153
}
128-
}
129154

155+
@Override
156+
public BuildVersion fromStream(StreamInput in) throws IOException {
157+
return new DefaultBuildVersion(in);
158+
}
159+
}
130160
}

‎server/src/main/java/org/elasticsearch/env/DefaultBuildVersion.java‎

Lines changed: 21 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,11 @@
1010
package org.elasticsearch.env;
1111

1212
import org.elasticsearch.Version;
13+
import org.elasticsearch.common.io.stream.StreamInput;
14+
import org.elasticsearch.common.io.stream.StreamOutput;
15+
import org.elasticsearch.xcontent.XContentBuilder;
1316

17+
import java.io.IOException;
1418
import java.util.Objects;
1519

1620
/**
@@ -28,7 +32,7 @@ final class DefaultBuildVersion extends BuildVersion {
2832

2933
public static BuildVersion CURRENT = new DefaultBuildVersion(Version.CURRENT.id());
3034

31-
private final Version version;
35+
final Version version;
3236

3337
DefaultBuildVersion(int versionId) {
3438
assert versionId >= 0 : "Release version IDs must be non-negative integers";
@@ -39,6 +43,10 @@ final class DefaultBuildVersion extends BuildVersion {
3943
this.version = Version.fromString(Objects.requireNonNull(version));
4044
}
4145

46+
DefaultBuildVersion(StreamInput in) throws IOException {
47+
this(in.readVInt());
48+
}
49+
4250
@Override
4351
public boolean onOrAfterMinimumCompatible() {
4452
return Version.CURRENT.minimumCompatibilityVersion().onOrBefore(version);
@@ -50,8 +58,18 @@ public boolean isFutureVersion() {
5058
}
5159

5260
@Override
53-
public int id() {
54-
return version.id();
61+
public String toNodeMetadata() {
62+
return Integer.toString(version.id());
63+
}
64+
65+
@Override
66+
public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
67+
return builder.value(version.id());
68+
}
69+
70+
@Override
71+
public void writeTo(StreamOutput out) throws IOException {
72+
out.writeVInt(version.id());
5573
}
5674

5775
@Override

‎server/src/main/java/org/elasticsearch/env/NodeMetadata.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -204,7 +204,7 @@ protected XContentBuilder newXContentBuilder(XContentType type, OutputStream str
204204
@Override
205205
public void toXContent(XContentBuilder builder, NodeMetadata nodeMetadata) throws IOException {
206206
builder.field(NODE_ID_KEY, nodeMetadata.nodeId);
207-
builder.field(NODE_VERSION_KEY, nodeMetadata.nodeVersion.id());
207+
builder.field(NODE_VERSION_KEY, nodeMetadata.nodeVersion);
208208
builder.field(OLDEST_INDEX_VERSION_KEY, nodeMetadata.oldestIndexVersion.id());
209209
}
210210

‎server/src/main/java/org/elasticsearch/gateway/PersistedClusterStateService.java‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -364,7 +364,7 @@ public static NodeMetadata nodeMetadata(Path... dataPaths) throws IOException {
364364
);
365365
} else if (nodeId == null) {
366366
nodeId = thisNodeId;
367-
version = BuildVersion.fromVersionId(Integer.parseInt(userData.get(NODE_VERSION_KEY)));
367+
version = BuildVersion.fromNodeMetadata(userData.get(NODE_VERSION_KEY));
368368
if (userData.containsKey(OLDEST_INDEX_VERSION_KEY)) {
369369
oldestIndexVersion = IndexVersion.fromId(Integer.parseInt(userData.get(OLDEST_INDEX_VERSION_KEY)));
370370
} else {
@@ -395,7 +395,7 @@ public static void overrideVersion(BuildVersion newVersion, Path... dataPaths) t
395395

396396
try (IndexWriter indexWriter = createIndexWriter(new NIOFSDirectory(dataPath.resolve(METADATA_DIRECTORY_NAME)), true)) {
397397
final Map<String, String> commitData = new HashMap<>(userData);
398-
commitData.put(NODE_VERSION_KEY, Integer.toString(newVersion.id()));
398+
commitData.put(NODE_VERSION_KEY, newVersion.toNodeMetadata());
399399
commitData.put(OVERRIDDEN_NODE_VERSION_KEY, Boolean.toString(true));
400400
indexWriter.setLiveCommitData(commitData.entrySet());
401401
indexWriter.commit();
@@ -852,7 +852,7 @@ void prepareCommit(
852852
final Map<String, String> commitData = Maps.newMapWithExpectedSize(COMMIT_DATA_SIZE);
853853
commitData.put(CURRENT_TERM_KEY, Long.toString(currentTerm));
854854
commitData.put(LAST_ACCEPTED_VERSION_KEY, Long.toString(lastAcceptedVersion));
855-
commitData.put(NODE_VERSION_KEY, Integer.toString(BuildVersion.current().id()));
855+
commitData.put(NODE_VERSION_KEY, BuildVersion.current().toNodeMetadata());
856856
commitData.put(OLDEST_INDEX_VERSION_KEY, Integer.toString(oldestIndexVersion.id()));
857857
commitData.put(NODE_ID_KEY, nodeId);
858858
commitData.put(CLUSTER_UUID_KEY, clusterUUID);

‎server/src/main/java/org/elasticsearch/internal/BuildExtension.java‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,8 +10,11 @@
1010
package org.elasticsearch.internal;
1111

1212
import org.elasticsearch.Build;
13+
import org.elasticsearch.common.io.stream.StreamInput;
1314
import org.elasticsearch.env.BuildVersion;
1415

16+
import java.io.IOException;
17+
1518
/**
1619
* Allows plugging in current build info.
1720
*/
@@ -39,8 +42,20 @@ default boolean hasReleaseVersioning() {
3942
*/
4043
BuildVersion fromVersionId(int versionId);
4144

45+
/**
46+
* Returns the {@link BuildVersion} as read from node metadata
47+
*/
48+
default BuildVersion fromNodeMetadata(String version) {
49+
return fromVersionId(Integer.parseInt(version));
50+
}
51+
4252
/**
4353
* Returns the {@link BuildVersion} for a given version string.
4454
*/
4555
BuildVersion fromString(String version);
56+
57+
/**
58+
* Reads a {@link BuildVersion} from the given stream
59+
*/
60+
BuildVersion fromStream(StreamInput in) throws IOException;
4661
}

‎server/src/main/java/org/elasticsearch/reservedstate/service/ReservedStateVersion.java‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -48,12 +48,12 @@ public static ReservedStateVersion parse(XContentParser parser) {
4848
}
4949

5050
public static ReservedStateVersion readFrom(StreamInput input) throws IOException {
51-
return new ReservedStateVersion(input.readLong(), BuildVersion.fromVersionId(input.readVInt()));
51+
return new ReservedStateVersion(input.readLong(), BuildVersion.fromStream(input));
5252
}
5353

5454
@Override
5555
public void writeTo(StreamOutput out) throws IOException {
56-
out.writeLong(version());
57-
out.writeVInt(buildVersion().id());
56+
out.writeLong(version);
57+
buildVersion.writeTo(out);
5858
}
5959
}

‎server/src/test/java/org/elasticsearch/env/BuildVersionTests.java‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -42,4 +42,8 @@ public void testIsFutureVersion() {
4242
assertFalse(afterMinCompat.isFutureVersion());
4343
assertTrue(futureVersion.isFutureVersion());
4444
}
45+
46+
public static BuildVersion increment(BuildVersion version) {
47+
return BuildVersion.fromVersionId(((DefaultBuildVersion) version).version.id() + 1);
48+
}
4549
}

‎server/src/test/java/org/elasticsearch/reservedstate/service/ReservedClusterStateServiceTests.java‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@
2626
import org.elasticsearch.common.settings.Settings;
2727
import org.elasticsearch.core.Releasable;
2828
import org.elasticsearch.env.BuildVersion;
29+
import org.elasticsearch.env.BuildVersionTests;
2930
import org.elasticsearch.reservedstate.ReservedClusterStateHandler;
3031
import org.elasticsearch.reservedstate.TransformState;
3132
import org.elasticsearch.reservedstate.action.ReservedClusterSettingsAction;
@@ -519,7 +520,7 @@ public void testCheckMetadataVersion() {
519520

520521
task = new ReservedStateUpdateTask(
521522
"test",
522-
new ReservedStateChunk(Map.of(), new ReservedStateVersion(124L, BuildVersion.fromVersionId(BuildVersion.current().id() + 1))),
523+
new ReservedStateChunk(Map.of(), new ReservedStateVersion(124L, BuildVersionTests.increment(BuildVersion.current()))),
523524
ReservedStateVersionCheck.HIGHER_VERSION_ONLY,
524525
Map.of(),
525526
List.of(),
@@ -529,7 +530,7 @@ public void testCheckMetadataVersion() {
529530
assertThat("Cluster state should not be modified", task.execute(state), sameInstance(state));
530531
task = new ReservedStateUpdateTask(
531532
"test",
532-
new ReservedStateChunk(Map.of(), new ReservedStateVersion(124L, BuildVersion.fromVersionId(BuildVersion.current().id() + 1))),
533+
new ReservedStateChunk(Map.of(), new ReservedStateVersion(124L, BuildVersionTests.increment(BuildVersion.current()))),
533534
ReservedStateVersionCheck.HIGHER_OR_SAME_VERSION,
534535
Map.of(),
535536
List.of(),

0 commit comments

Comments
 (0)