Skip to content

Commit b4ce564

Browse files
HyunSangHanHyunSangHan
committed
Use boolean isOpenSearch instead of String distribution in RemoteVersion
Simplifies the RemoteVersion class by replacing the distribution string field with a boolean flag, making the code more readable and maintainable. Signed-off-by: Hyunsang Han <[email protected]>
1 parent cea7ab1 commit b4ce564

File tree

3 files changed

+61
-139
lines changed

3 files changed

+61
-139
lines changed

‎modules/reindex/src/main/java/org/opensearch/index/reindex/remote/RemoteResponseParsers.java‎

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -300,12 +300,11 @@ public void setCausedBy(Throwable causedBy) {
300300
a -> (RemoteVersion) a[0]
301301
);
302302
static {
303-
ConstructingObjectParser<RemoteVersion, MediaType> versionParser = new ConstructingObjectParser<>("version", true, a -> {
304-
String distribution = (String) a[0];
305-
String number = (String) a[1];
306-
String cleanNumber = number.replace("-SNAPSHOT", "").replaceFirst("-(alpha\\d+|beta\\d+|rc\\d+)", "");
307-
return RemoteVersion.fromString(cleanNumber, distribution);
308-
});
303+
ConstructingObjectParser<RemoteVersion, MediaType> versionParser = new ConstructingObjectParser<>(
304+
"version",
305+
true,
306+
a -> RemoteVersion.fromString(((String) a[1]).replace("-SNAPSHOT", "").replaceFirst("-(alpha\\d+|beta\\d+|rc\\d+)", ""), a[0] != null)
307+
);
309308
versionParser.declareStringOrNull(optionalConstructorArg(), new ParseField("distribution"));
310309
versionParser.declareString(constructorArg(), new ParseField("number"));
311310
MAIN_ACTION_PARSER.declareObject(constructorArg(), versionParser, new ParseField("version"));

‎modules/reindex/src/main/java/org/opensearch/index/reindex/remote/RemoteVersion.java‎

Lines changed: 21 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,6 @@
88

99
package org.opensearch.index.reindex.remote;
1010

11-
import java.util.Locale;
1211
import java.util.Objects;
1312

1413
/**
@@ -21,37 +20,37 @@ public final class RemoteVersion implements Comparable<RemoteVersion> {
2120
private final int major;
2221
private final int minor;
2322
private final int revision;
24-
private final String distribution;
23+
final boolean isOpenSearch;
2524

2625
// Common version constants for backward compatibility
27-
public static final RemoteVersion ELASTICSEARCH_0_20_5 = new RemoteVersion(0, 20, 5, null);
28-
public static final RemoteVersion ELASTICSEARCH_0_90_13 = new RemoteVersion(0, 90, 13, null);
29-
public static final RemoteVersion ELASTICSEARCH_1_0_0 = new RemoteVersion(1, 0, 0, null);
30-
public static final RemoteVersion ELASTICSEARCH_1_7_5 = new RemoteVersion(1, 7, 5, null);
31-
public static final RemoteVersion ELASTICSEARCH_2_0_0 = new RemoteVersion(2, 0, 0, null);
32-
public static final RemoteVersion ELASTICSEARCH_2_1_0 = new RemoteVersion(2, 1, 0, null);
33-
public static final RemoteVersion ELASTICSEARCH_2_3_3 = new RemoteVersion(2, 3, 3, null);
34-
public static final RemoteVersion ELASTICSEARCH_5_0_0 = new RemoteVersion(5, 0, 0, null);
35-
public static final RemoteVersion ELASTICSEARCH_6_0_0 = new RemoteVersion(6, 0, 0, null);
36-
public static final RemoteVersion ELASTICSEARCH_6_3_0 = new RemoteVersion(6, 3, 0, null);
37-
public static final RemoteVersion ELASTICSEARCH_7_0_0 = new RemoteVersion(7, 0, 0, null);
26+
public static final RemoteVersion ELASTICSEARCH_0_20_5 = new RemoteVersion(0, 20, 5, false);
27+
public static final RemoteVersion ELASTICSEARCH_0_90_13 = new RemoteVersion(0, 90, 13, false);
28+
public static final RemoteVersion ELASTICSEARCH_1_0_0 = new RemoteVersion(1, 0, 0, false);
29+
public static final RemoteVersion ELASTICSEARCH_1_7_5 = new RemoteVersion(1, 7, 5, false);
30+
public static final RemoteVersion ELASTICSEARCH_2_0_0 = new RemoteVersion(2, 0, 0, false);
31+
public static final RemoteVersion ELASTICSEARCH_2_1_0 = new RemoteVersion(2, 1, 0, false);
32+
public static final RemoteVersion ELASTICSEARCH_2_3_3 = new RemoteVersion(2, 3, 3, false);
33+
public static final RemoteVersion ELASTICSEARCH_5_0_0 = new RemoteVersion(5, 0, 0, false);
34+
public static final RemoteVersion ELASTICSEARCH_6_0_0 = new RemoteVersion(6, 0, 0, false);
35+
public static final RemoteVersion ELASTICSEARCH_6_3_0 = new RemoteVersion(6, 3, 0, false);
36+
public static final RemoteVersion ELASTICSEARCH_7_0_0 = new RemoteVersion(7, 0, 0, false);
3837

3938
// OpenSearch versions
40-
public static final RemoteVersion OPENSEARCH_1_0_0 = new RemoteVersion(1, 0, 0, "opensearch");
41-
public static final RemoteVersion OPENSEARCH_2_0_0 = new RemoteVersion(2, 0, 0, "opensearch");
42-
public static final RemoteVersion OPENSEARCH_3_1_0 = new RemoteVersion(3, 1, 0, "opensearch");
39+
public static final RemoteVersion OPENSEARCH_1_0_0 = new RemoteVersion(1, 0, 0, true);
40+
public static final RemoteVersion OPENSEARCH_2_0_0 = new RemoteVersion(2, 0, 0, true);
41+
public static final RemoteVersion OPENSEARCH_3_1_0 = new RemoteVersion(3, 1, 0, true);
4342

44-
public RemoteVersion(int major, int minor, int revision, String distribution) {
43+
public RemoteVersion(int major, int minor, int revision, boolean isOpenSearch) {
4544
this.major = major;
4645
this.minor = minor;
4746
this.revision = revision;
48-
this.distribution = distribution != null ? distribution.toLowerCase(Locale.ROOT) : "elasticsearch";
47+
this.isOpenSearch = isOpenSearch;
4948
}
5049

5150
/**
5251
* Parse version string like "7.10.2" or "1.0.0"
5352
*/
54-
public static RemoteVersion fromString(String version, String distribution) {
53+
public static RemoteVersion fromString(String version, boolean isOpenSearch) {
5554
if (version == null || version.trim().isEmpty()) {
5655
throw new IllegalArgumentException("Version string cannot be null or empty");
5756
}
@@ -69,16 +68,12 @@ public static RemoteVersion fromString(String version, String distribution) {
6968
int minor = Integer.parseInt(parts[1]);
7069
int revision = parts.length > 2 ? Integer.parseInt(parts[2]) : 0;
7170

72-
return new RemoteVersion(major, minor, revision, distribution);
71+
return new RemoteVersion(major, minor, revision, isOpenSearch);
7372
} catch (NumberFormatException e) {
7473
throw new IllegalArgumentException("Invalid version format: " + version, e);
7574
}
7675
}
7776

78-
public static RemoteVersion fromString(String version) {
79-
return fromString(version, "elasticsearch");
80-
}
81-
8277
public boolean before(RemoteVersion other) {
8378
return this.compareTo(other) < 0;
8479
}
@@ -95,14 +90,6 @@ public boolean after(RemoteVersion other) {
9590
return this.compareTo(other) > 0;
9691
}
9792

98-
public boolean isOpenSearch() {
99-
return "opensearch".equals(distribution);
100-
}
101-
102-
public boolean isElasticsearch() {
103-
return "elasticsearch".equals(distribution);
104-
}
105-
10693
@Override
10794
public int compareTo(RemoteVersion other) {
10895
if (other == null) {
@@ -131,12 +118,12 @@ public boolean equals(Object obj) {
131118
return false;
132119
}
133120
RemoteVersion that = (RemoteVersion) obj;
134-
return major == that.major && minor == that.minor && revision == that.revision && Objects.equals(distribution, that.distribution);
121+
return major == that.major && minor == that.minor && revision == that.revision && Objects.equals(isOpenSearch, that.isOpenSearch);
135122
}
136123

137124
@Override
138125
public int hashCode() {
139-
return Objects.hash(major, minor, revision, distribution);
126+
return Objects.hash(major, minor, revision, isOpenSearch);
140127
}
141128

142129
@Override
@@ -155,8 +142,4 @@ public int getMinor() {
155142
public int getRevision() {
156143
return revision;
157144
}
158-
159-
public String getDistribution() {
160-
return distribution;
161-
}
162145
}

‎modules/reindex/src/test/java/org/opensearch/index/reindex/remote/RemoteVersionTests.java‎

Lines changed: 35 additions & 95 deletions
Original file line numberDiff line numberDiff line change
@@ -38,69 +38,55 @@ public void testVersionComparison() {
3838
assertTrue(RemoteVersion.ELASTICSEARCH_2_0_0.compareTo(RemoteVersion.ELASTICSEARCH_1_0_0) > 0);
3939

4040
// Test OpenSearch vs Elasticsearch versions with same numbers
41-
assertEquals(0, RemoteVersion.ELASTICSEARCH_2_0_0.compareTo(RemoteVersion.OPENSEARCH_2_0_0)); // Same version numbers should be equal in
42-
// comparison
41+
assertEquals(0, RemoteVersion.ELASTICSEARCH_2_0_0.compareTo(RemoteVersion.OPENSEARCH_2_0_0));
4342
}
4443

4544
public void testVersionFromString() {
4645
// Test basic version parsing
47-
RemoteVersion version = RemoteVersion.fromString("7.10.2");
46+
RemoteVersion version = RemoteVersion.fromString("7.10.2", false);
4847
assertEquals(7, version.getMajor());
4948
assertEquals(10, version.getMinor());
5049
assertEquals(2, version.getRevision());
51-
assertEquals("elasticsearch", version.getDistribution());
52-
53-
// Test version with distribution
54-
RemoteVersion opensearchVersion = RemoteVersion.fromString("1.0.0", "opensearch");
55-
assertEquals(1, opensearchVersion.getMajor());
56-
assertEquals(0, opensearchVersion.getMinor());
57-
assertEquals(0, opensearchVersion.getRevision());
58-
assertEquals("opensearch", opensearchVersion.getDistribution());
59-
60-
// Test version without revision
61-
RemoteVersion versionNoRevision = RemoteVersion.fromString("2.1");
62-
assertEquals(2, versionNoRevision.getMajor());
63-
assertEquals(1, versionNoRevision.getMinor());
64-
assertEquals(0, versionNoRevision.getRevision());
50+
assertFalse(version.isOpenSearch);
6551

6652
// Test version with snapshot qualifier
67-
RemoteVersion snapshotVersion = RemoteVersion.fromString("2.0.0-SNAPSHOT");
53+
RemoteVersion snapshotVersion = RemoteVersion.fromString("2.0.0-SNAPSHOT", false);
6854
assertEquals(2, snapshotVersion.getMajor());
6955
assertEquals(0, snapshotVersion.getMinor());
7056
assertEquals(0, snapshotVersion.getRevision());
7157

7258
// Test version with pre-release qualifiers
73-
RemoteVersion alphaVersion = RemoteVersion.fromString("2.0.0-alpha1");
59+
RemoteVersion alphaVersion = RemoteVersion.fromString("2.0.0-alpha1", false);
7460
assertEquals(2, alphaVersion.getMajor());
7561
assertEquals(0, alphaVersion.getMinor());
7662
assertEquals(0, alphaVersion.getRevision());
7763

78-
RemoteVersion betaVersion = RemoteVersion.fromString("2.0.0-beta2");
64+
RemoteVersion betaVersion = RemoteVersion.fromString("2.0.0-beta2", false);
7965
assertEquals(2, betaVersion.getMajor());
8066
assertEquals(0, betaVersion.getMinor());
8167
assertEquals(0, betaVersion.getRevision());
8268

83-
RemoteVersion rcVersion = RemoteVersion.fromString("2.0.0-rc1");
69+
RemoteVersion rcVersion = RemoteVersion.fromString("2.0.0-rc1", false);
8470
assertEquals(2, rcVersion.getMajor());
8571
assertEquals(0, rcVersion.getMinor());
8672
assertEquals(0, rcVersion.getRevision());
8773
}
8874

8975
public void testInvalidVersionFromString() {
9076
// Test null version
91-
Exception e = expectThrows(IllegalArgumentException.class, () -> RemoteVersion.fromString(null));
77+
Exception e = expectThrows(IllegalArgumentException.class, () -> RemoteVersion.fromString(null, false));
9278
assertTrue(e.getMessage().contains("Version string cannot be null or empty"));
9379

9480
// Test empty version
95-
e = expectThrows(IllegalArgumentException.class, () -> RemoteVersion.fromString(""));
81+
e = expectThrows(IllegalArgumentException.class, () -> RemoteVersion.fromString("", false));
9682
assertTrue(e.getMessage().contains("Version string cannot be null or empty"));
9783

9884
// Test invalid format
99-
e = expectThrows(IllegalArgumentException.class, () -> RemoteVersion.fromString("invalid"));
85+
e = expectThrows(IllegalArgumentException.class, () -> RemoteVersion.fromString("invalid", false));
10086
assertTrue(e.getMessage().contains("Invalid version format"));
10187

10288
// Test non-numeric version
103-
e = expectThrows(IllegalArgumentException.class, () -> RemoteVersion.fromString("a.b.c"));
89+
e = expectThrows(IllegalArgumentException.class, () -> RemoteVersion.fromString("a.b.c", false));
10490
assertTrue(e.getMessage().contains("Invalid version format"));
10591
}
10692

@@ -109,28 +95,28 @@ public void testVersionConstants() {
10995
assertEquals(0, RemoteVersion.ELASTICSEARCH_0_20_5.getMajor());
11096
assertEquals(20, RemoteVersion.ELASTICSEARCH_0_20_5.getMinor());
11197
assertEquals(5, RemoteVersion.ELASTICSEARCH_0_20_5.getRevision());
112-
assertEquals("elasticsearch", RemoteVersion.ELASTICSEARCH_0_20_5.getDistribution());
98+
assertFalse(RemoteVersion.ELASTICSEARCH_0_20_5.isOpenSearch);
11399

114100
assertEquals(7, RemoteVersion.ELASTICSEARCH_7_0_0.getMajor());
115101
assertEquals(0, RemoteVersion.ELASTICSEARCH_7_0_0.getMinor());
116102
assertEquals(0, RemoteVersion.ELASTICSEARCH_7_0_0.getRevision());
117-
assertEquals("elasticsearch", RemoteVersion.ELASTICSEARCH_7_0_0.getDistribution());
103+
assertFalse(RemoteVersion.ELASTICSEARCH_7_0_0.isOpenSearch);
118104

119105
// Test OpenSearch versions
120106
assertEquals(1, RemoteVersion.OPENSEARCH_1_0_0.getMajor());
121107
assertEquals(0, RemoteVersion.OPENSEARCH_1_0_0.getMinor());
122108
assertEquals(0, RemoteVersion.OPENSEARCH_1_0_0.getRevision());
123-
assertEquals("opensearch", RemoteVersion.OPENSEARCH_1_0_0.getDistribution());
109+
assertTrue(RemoteVersion.OPENSEARCH_1_0_0.isOpenSearch);
124110

125111
assertEquals(2, RemoteVersion.OPENSEARCH_2_0_0.getMajor());
126112
assertEquals(0, RemoteVersion.OPENSEARCH_2_0_0.getMinor());
127113
assertEquals(0, RemoteVersion.OPENSEARCH_2_0_0.getRevision());
128-
assertEquals("opensearch", RemoteVersion.OPENSEARCH_2_0_0.getDistribution());
114+
assertTrue(RemoteVersion.OPENSEARCH_2_0_0.isOpenSearch);
129115

130116
assertEquals(3, RemoteVersion.OPENSEARCH_3_1_0.getMajor());
131117
assertEquals(1, RemoteVersion.OPENSEARCH_3_1_0.getMinor());
132118
assertEquals(0, RemoteVersion.OPENSEARCH_3_1_0.getRevision());
133-
assertEquals("opensearch", RemoteVersion.OPENSEARCH_3_1_0.getDistribution());
119+
assertTrue(RemoteVersion.OPENSEARCH_3_1_0.isOpenSearch);
134120
}
135121

136122
public void testVersionOrdering() {
@@ -147,36 +133,15 @@ public void testVersionOrdering() {
147133
assertTrue(RemoteVersion.ELASTICSEARCH_6_3_0.before(RemoteVersion.ELASTICSEARCH_7_0_0));
148134
}
149135

150-
public void testDistributionChecks() {
151-
// Test isElasticsearch()
152-
assertTrue(RemoteVersion.ELASTICSEARCH_7_0_0.isElasticsearch());
153-
assertFalse(RemoteVersion.OPENSEARCH_1_0_0.isElasticsearch());
154-
155-
// Test isOpenSearch()
156-
assertTrue(RemoteVersion.OPENSEARCH_1_0_0.isOpenSearch());
157-
assertFalse(RemoteVersion.ELASTICSEARCH_7_0_0.isOpenSearch());
158-
159-
// Test custom distribution
160-
RemoteVersion customVersion = new RemoteVersion(1, 0, 0, "custom");
161-
assertFalse(customVersion.isElasticsearch());
162-
assertFalse(customVersion.isOpenSearch());
163-
assertEquals("custom", customVersion.getDistribution());
164-
165-
// Test null distribution defaults to elasticsearch
166-
RemoteVersion nullDistVersion = new RemoteVersion(1, 0, 0, null);
167-
assertTrue(nullDistVersion.isElasticsearch());
168-
assertFalse(nullDistVersion.isOpenSearch());
169-
}
170-
171136
public void testEqualsAndHashCode() {
172-
RemoteVersion version1 = new RemoteVersion(2, 0, 0, null);
173-
RemoteVersion version2 = new RemoteVersion(2, 0, 0, null);
174-
RemoteVersion version3 = new RemoteVersion(2, 0, 0, "opensearch");
175-
RemoteVersion version4 = new RemoteVersion(2, 0, 1, null);
137+
RemoteVersion version1 = new RemoteVersion(2, 0, 0, false);
138+
RemoteVersion version2 = new RemoteVersion(2, 0, 0, false);
139+
RemoteVersion version3 = new RemoteVersion(2, 0, 0, true);
140+
RemoteVersion version4 = new RemoteVersion(2, 0, 1, false);
176141

177142
// Test equals
178143
assertEquals(version1, version2);
179-
assertNotEquals(version1, version3); // Different distribution
144+
assertNotEquals(version1, version3); // Different isOpenSearch
180145
assertNotEquals(version1, version4); // Different revision
181146
assertNotEquals(version1, null);
182147
assertNotEquals(version1, "not a version");
@@ -193,7 +158,7 @@ public void testEqualsAndHashCode() {
193158
* Test equals method
194159
*/
195160
public void testEqualsMethodAllBranches() {
196-
RemoteVersion baseVersion = new RemoteVersion(2, 1, 3, "elasticsearch");
161+
RemoteVersion baseVersion = new RemoteVersion(2, 1, 3, false);
197162

198163
// Test same object (reflexivity)
199164
assertEquals(baseVersion, baseVersion);
@@ -205,44 +170,31 @@ public void testEqualsMethodAllBranches() {
205170
assertNotEquals(baseVersion, "not a version object");
206171

207172
// Test different major version
208-
RemoteVersion differentMajor = new RemoteVersion(1, 1, 3, "elasticsearch");
173+
RemoteVersion differentMajor = new RemoteVersion(1, 1, 3, false);
209174
assertNotEquals(baseVersion, differentMajor);
210175

211176
// Test different minor version
212-
RemoteVersion differentMinor = new RemoteVersion(2, 0, 3, "elasticsearch");
177+
RemoteVersion differentMinor = new RemoteVersion(2, 0, 3, false);
213178
assertNotEquals(baseVersion, differentMinor);
214179

215180
// Test different revision
216-
RemoteVersion differentRevision = new RemoteVersion(2, 1, 2, "elasticsearch");
181+
RemoteVersion differentRevision = new RemoteVersion(2, 1, 2, false);
217182
assertNotEquals(baseVersion, differentRevision);
218183

219-
// Test different distribution (null vs non-null) - null becomes "elasticsearch", so they are equal
220-
RemoteVersion nullDistribution = new RemoteVersion(2, 1, 3, null);
221-
assertEquals(baseVersion, nullDistribution); // null distribution becomes "elasticsearch"
222-
223-
// Test different distribution (non-null vs non-null)
224-
RemoteVersion opensearchDistribution = new RemoteVersion(2, 1, 3, "opensearch");
225-
assertNotEquals(baseVersion, opensearchDistribution);
226-
227-
// Test same distribution (both null)
228-
RemoteVersion nullDist1 = new RemoteVersion(2, 1, 3, null);
229-
RemoteVersion nullDist2 = new RemoteVersion(2, 1, 3, null);
230-
assertEquals(nullDist1, nullDist2);
184+
// Test different isOpenSearch
185+
RemoteVersion differentIsOpenSearch = new RemoteVersion(2, 1, 3, true);
186+
assertNotEquals(baseVersion, differentIsOpenSearch);
231187

232188
// Test identical version (all fields match)
233-
RemoteVersion identical = new RemoteVersion(2, 1, 3, "elasticsearch");
189+
RemoteVersion identical = new RemoteVersion(2, 1, 3, false);
234190
assertEquals(baseVersion, identical);
235-
236-
// Test case sensitivity in distribution - both become lowercase, so they are equal
237-
RemoteVersion upperCaseDist = new RemoteVersion(2, 1, 3, "ELASTICSEARCH");
238-
assertEquals(baseVersion, upperCaseDist); // Both become "elasticsearch" due to toLowerCase()
239191
}
240192

241193
public void testToString() {
242-
RemoteVersion version = new RemoteVersion(2, 1, 3, null);
194+
RemoteVersion version = new RemoteVersion(2, 1, 3, false);
243195
assertEquals("2.1.3", version.toString());
244196

245-
RemoteVersion opensearchVersion = new RemoteVersion(1, 0, 0, "opensearch");
197+
RemoteVersion opensearchVersion = new RemoteVersion(1, 0, 0, true);
246198
assertEquals("1.0.0", opensearchVersion.toString());
247199
}
248200

@@ -253,21 +205,9 @@ public void testCompareToWithNull() {
253205

254206
public void testVersionConstantsAreCorrect() {
255207
// Verify that our constants match what fromString would produce
256-
assertEquals(RemoteVersion.ELASTICSEARCH_2_0_0, RemoteVersion.fromString("2.0.0"));
257-
assertEquals(RemoteVersion.ELASTICSEARCH_7_0_0, RemoteVersion.fromString("7.0.0"));
258-
assertEquals(RemoteVersion.OPENSEARCH_1_0_0, RemoteVersion.fromString("1.0.0", "opensearch"));
259-
assertEquals(RemoteVersion.OPENSEARCH_2_0_0, RemoteVersion.fromString("2.0.0", "opensearch"));
260-
}
261-
262-
public void testCaseInsensitiveDistribution() {
263-
RemoteVersion version1 = new RemoteVersion(1, 0, 0, null);
264-
RemoteVersion version2 = new RemoteVersion(1, 0, 0, "elasticsearch");
265-
RemoteVersion version3 = new RemoteVersion(1, 0, 0, "ElasticSearch");
266-
267-
assertEquals(version1, version2);
268-
assertEquals(version1, version3);
269-
assertTrue(version1.isElasticsearch());
270-
assertTrue(version2.isElasticsearch());
271-
assertTrue(version3.isElasticsearch());
208+
assertEquals(RemoteVersion.ELASTICSEARCH_2_0_0, RemoteVersion.fromString("2.0.0", false));
209+
assertEquals(RemoteVersion.ELASTICSEARCH_7_0_0, RemoteVersion.fromString("7.0.0", false));
210+
assertEquals(RemoteVersion.OPENSEARCH_1_0_0, RemoteVersion.fromString("1.0.0", true));
211+
assertEquals(RemoteVersion.OPENSEARCH_2_0_0, RemoteVersion.fromString("2.0.0", true));
272212
}
273213
}

0 commit comments

Comments
 (0)