Skip to content
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ testfixtures_shared/
# Generated
checkstyle_ide.xml
x-pack/plugin/esql/src/main/generated-src/generated/
server/src/main/resources/transport/defined/manifest.txt
server/src/main/resources/transport/definitions/manifest.txt

# JEnv
.java-version
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,12 @@ class TransportVersionManagementPluginFuncTest extends AbstractGradleFuncTest {
}
}

def definedTransportVersion(String name, String ids) {
javaResource("myserver", "transport/defined/" + name + ".csv", ids)
def namedTransportVersion(String name, String ids) {
javaResource("myserver", "transport/definitions/named/" + name + ".csv", ids)
}

def initialTransportVersion(String name, String id) {
javaResource("myserver", "transport/definitions/initial/" + name + ".csv", id)
}

def definedAndUsedTransportVersion(String name, String ids) {
Expand All @@ -54,7 +58,7 @@ class TransportVersionManagementPluginFuncTest extends AbstractGradleFuncTest {
javaSource("myserver", "org.elasticsearch", classname, "", """
static final TransportVersion usage = TransportVersion.fromName("${name}");
""")
definedTransportVersion(name, ids)
namedTransportVersion(name, ids)
}

def latestTransportVersion(String branch, String name, String id) {
Expand Down Expand Up @@ -95,8 +99,9 @@ class TransportVersionManagementPluginFuncTest extends AbstractGradleFuncTest {
apply plugin: 'elasticsearch.transport-version-references'
apply plugin: 'elasticsearch.transport-version-resources'
"""
definedTransportVersion("existing_91", "8012000")
definedTransportVersion("existing_92", "8123000,8012001")
namedTransportVersion("existing_91", "8012000")
namedTransportVersion("existing_92", "8123000,8012001")
initialTransportVersion("initial_9_0_0", "8000000")
latestTransportVersion("9.2", "existing_92", "8123000")
latestTransportVersion("9.1", "existing_92", "8012001")
// a mock version of TransportVersion, just here so we can compile Dummy.java et al
Expand Down Expand Up @@ -148,12 +153,12 @@ class TransportVersionManagementPluginFuncTest extends AbstractGradleFuncTest {

def "references must be defined"() {
given:
definedTransportVersion("not_used", "1000000")
namedTransportVersion("not_used", "1000000")
when:
def result = validateDefinitionsFails()
then:
assertDefinitionsFailure(result, "Transport version definition file " +
"[myserver/src/main/resources/transport/defined/not_used.csv] is not referenced")
"[myserver/src/main/resources/transport/definitions/named/not_used.csv] is not referenced")
}

def "names must be lowercase alphanum or underscore"() {
Expand All @@ -163,7 +168,7 @@ class TransportVersionManagementPluginFuncTest extends AbstractGradleFuncTest {
def result = validateDefinitionsFails()
then:
assertDefinitionsFailure(result, "Transport version definition file " +
"[myserver/src/main/resources/transport/defined/${name}.csv] does not have a valid name, " +
"[myserver/src/main/resources/transport/definitions/named/${name}.csv] does not have a valid name, " +
"must be lowercase alphanumeric and underscore")

where:
Expand All @@ -177,7 +182,7 @@ class TransportVersionManagementPluginFuncTest extends AbstractGradleFuncTest {
def result = validateDefinitionsFails()
then:
assertDefinitionsFailure(result, "Transport version definition file " +
"[myserver/src/main/resources/transport/defined/empty.csv] does not contain any ids")
"[myserver/src/main/resources/transport/definitions/named/empty.csv] does not contain any ids")
}

def "definitions have ids in descending order"() {
Expand All @@ -187,7 +192,7 @@ class TransportVersionManagementPluginFuncTest extends AbstractGradleFuncTest {
def result = validateDefinitionsFails()
then:
assertDefinitionsFailure(result, "Transport version definition file " +
"[myserver/src/main/resources/transport/defined/out_of_order.csv] does not have ordered ids")
"[myserver/src/main/resources/transport/definitions/named/out_of_order.csv] does not have ordered ids")
}

def "definition ids are unique"() {
Expand All @@ -197,8 +202,8 @@ class TransportVersionManagementPluginFuncTest extends AbstractGradleFuncTest {
def result = validateDefinitionsFails()
then:
assertDefinitionsFailure(result, "Transport version definition file " +
"[myserver/src/main/resources/transport/defined/existing_92.csv] contains id 8123000 already defined in " +
"[myserver/src/main/resources/transport/defined/duplicate.csv]")
"[myserver/src/main/resources/transport/definitions/named/existing_92.csv] contains id 8123000 already defined in " +
"[myserver/src/main/resources/transport/definitions/named/duplicate.csv]")
}

def "definitions have bwc ids with non-zero patch part"() {
Expand All @@ -208,27 +213,27 @@ class TransportVersionManagementPluginFuncTest extends AbstractGradleFuncTest {
def result = validateDefinitionsFails()
then:
assertDefinitionsFailure(result, "Transport version definition file " +
"[myserver/src/main/resources/transport/defined/patched.csv] contains bwc id [8100000] with a patch part of 0")
"[myserver/src/main/resources/transport/definitions/named/patched.csv] contains bwc id [8100000] with a patch part of 0")
}

def "definitions have primary ids which cannot change"() {
given:
definedTransportVersion("existing_92", "8500000")
namedTransportVersion("existing_92", "8500000")
when:
def result = validateDefinitionsFails()
then:
assertDefinitionsFailure(result, "Transport version definition file " +
"[myserver/src/main/resources/transport/defined/existing_92.csv] has modified primary id from 8123000 to 8500000")
"[myserver/src/main/resources/transport/definitions/named/existing_92.csv] has modified primary id from 8123000 to 8500000")
}

def "cannot change committed ids to a branch"() {
given:
definedTransportVersion("existing_92", "8123000,8012002")
namedTransportVersion("existing_92", "8123000,8012002")
when:
def result = validateDefinitionsFails()
then:
assertDefinitionsFailure(result, "Transport version definition file " +
"[myserver/src/main/resources/transport/defined/existing_92.csv] modifies existing patch id from 8012001 to 8012002")
"[myserver/src/main/resources/transport/definitions/named/existing_92.csv] modifies existing patch id from 8012001 to 8012002")
}

def "latest files must reference defined name"() {
Expand All @@ -249,7 +254,7 @@ class TransportVersionManagementPluginFuncTest extends AbstractGradleFuncTest {
then:
assertDefinitionsFailure(result, "Latest transport version file " +
"[myserver/src/main/resources/transport/latest/9.2.csv] has id 8124000 which is not in definition " +
"[myserver/src/main/resources/transport/defined/existing_92.csv]")
"[myserver/src/main/resources/transport/definitions/named/existing_92.csv]")
}

def "latest files have latest id within base"() {
Expand Down Expand Up @@ -296,6 +301,6 @@ class TransportVersionManagementPluginFuncTest extends AbstractGradleFuncTest {
def result = validateDefinitionsFails()
then:
assertDefinitionsFailure(result, "Transport version definition file " +
"[myserver/src/main/resources/transport/defined/patch.csv] has patch version 8015001 as primary id")
"[myserver/src/main/resources/transport/definitions/named/patch.csv] has patch version 8015001 as primary id")
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,11 @@
import org.gradle.api.tasks.TaskAction;

import java.io.IOException;
import java.nio.file.FileVisitResult;
import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.SimpleFileVisitor;
import java.nio.file.attribute.BasicFileAttributes;

public abstract class GenerateTransportVersionManifestTask extends DefaultTask {
@InputDirectory
Expand All @@ -32,15 +35,14 @@ public void generateTransportVersionManifest() throws IOException {
Path definitionsDir = getDefinitionsDirectory().get().getAsFile().toPath();
Path manifestFile = getManifestFile().get().getAsFile().toPath();
try (var writer = Files.newBufferedWriter(manifestFile)) {
try (var stream = Files.list(definitionsDir)) {
for (String filename : stream.map(p -> p.getFileName().toString()).toList()) {
if (filename.equals(manifestFile.getFileName().toString())) {
// don't list self
continue;
}
writer.write(filename + "\n");
Files.walkFileTree(definitionsDir, new SimpleFileVisitor<>() {
@Override
public FileVisitResult visitFile(Path path, BasicFileAttributes attrs) throws IOException {
String subPath = definitionsDir.relativize(path).toString();
writer.write(subPath + "\n");
return FileVisitResult.CONTINUE;
}
}
});
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ public void apply(Project project) {
t.getManifestFile().set(project.getLayout().getBuildDirectory().file("generated-resources/manifest.txt"));
});
project.getTasks().named(JavaPlugin.PROCESS_RESOURCES_TASK_NAME, Copy.class).configure(t -> {
t.into("transport/defined", c -> c.from(generateManifestTask));
t.into("transport/definitions", c -> c.from(generateManifestTask));
});
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
class TransportVersionUtils {

static Path definitionFilePath(Directory resourcesDirectory, String name) {
return getDefinitionsDirectory(resourcesDirectory).getAsFile().toPath().resolve(name + ".csv");
return getDefinitionsDirectory(resourcesDirectory).getAsFile().toPath().resolve("named/" + name + ".csv");
}

static Path latestFilePath(Directory resourcesDirectory, String name) {
Expand All @@ -38,7 +38,7 @@ static TransportVersionLatest readLatestFile(Path file) throws IOException {
}

static Directory getDefinitionsDirectory(Directory resourcesDirectory) {
return resourcesDirectory.dir("defined");
return resourcesDirectory.dir("definitions");
}

static Directory getLatestDirectory(Directory resourcesDirectory) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ public void validateTransportVersions() throws IOException {
final Predicate<String> referenceChecker;
if (getDefinitionsDirectory().isPresent()) {
Path definitionsDir = getDefinitionsDirectory().getAsFile().get().toPath();
referenceChecker = (name) -> Files.exists(definitionsDir.resolve(name + ".csv"));
referenceChecker = (name) -> Files.exists(definitionsDir.resolve("named/" + name + ".csv"));
} else {
referenceChecker = (name) -> false;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ public ValidateTransportVersionResourcesTask(ExecOperations execOperations) {
@TaskAction
public void validateTransportVersions() throws IOException {
Path resourcesDir = getResourcesDirectory().getAsFile().get().toPath();
Path definitionsDir = resourcesDir.resolve("defined");
Path definitionsDir = resourcesDir.resolve("definitions");
Path latestDir = resourcesDir.resolve("latest");

// first check which resource files already exist in main
Expand All @@ -107,9 +107,11 @@ public void validateTransportVersions() throws IOException {
// now load all definitions, do some validation and record them by various keys for later quick lookup
// NOTE: this must run after loading referenced names and existing definitions
// NOTE: this is sorted so that the order of cross validation is deterministic
try (var definitionsStream = Files.list(definitionsDir).sorted()) {
for (var definitionFile : definitionsStream.toList()) {
recordAndValidateDefinition(readDefinitionFile(definitionFile));
for (String subDir : List.of("initial", "named")) {
try (var definitionsStream = Files.list(definitionsDir.resolve(subDir)).sorted()) {
for (var definitionFile : definitionsStream.toList()) {
recordAndValidateDefinition(readDefinitionFile(definitionFile));
}
}
}

Expand Down
40 changes: 25 additions & 15 deletions server/src/main/java/org/elasticsearch/TransportVersion.java
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@
import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Objects;
Expand Down Expand Up @@ -113,6 +112,7 @@ public static TransportVersion fromBufferedReader(
String component,
String path,
boolean nameInFile,
boolean ignoreName,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: maybe flip this to positive logic, like "recordName" or "captureName"?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed to isNamed

BufferedReader bufferedReader,
Integer latest
) {
Expand All @@ -128,7 +128,14 @@ public static TransportVersion fromBufferedReader(
if (parts.length < (nameInFile ? 2 : 1)) {
throw new IllegalStateException("invalid transport version file format [" + toComponentPath(component, path) + "]");
}
String name = nameInFile ? parts[0] : path.substring(path.lastIndexOf('/') + 1, path.length() - 4);
String name = null;
if (ignoreName == false) {
if (nameInFile) {
name = parts[0];
} else {
name = path.substring(path.lastIndexOf('/') + 1, path.length() - 4);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optional: I'm not a fan of "magic numbers" (4) here or elsewhere, I'd rather this reference a constant if only purely to improve readability and maintainability. Not blocking - your call.

}
}
List<Integer> ids = new ArrayList<>();
for (int i = nameInFile ? 1 : 0; i < parts.length; ++i) {
try {
Expand Down Expand Up @@ -156,7 +163,7 @@ public static TransportVersion fromBufferedReader(
}
}

public static Map<String, TransportVersion> collectFromInputStreams(
public static List<TransportVersion> collectFromInputStreams(
String component,
Function<String, InputStream> nameToStream,
String latestFileName
Expand All @@ -165,32 +172,32 @@ public static Map<String, TransportVersion> collectFromInputStreams(
component,
"/transport/latest/" + latestFileName,
nameToStream,
(c, p, br) -> fromBufferedReader(c, p, true, br, Integer.MAX_VALUE)
(c, p, br) -> fromBufferedReader(c, p, true, false, br, Integer.MAX_VALUE)
);
if (latest != null) {
List<String> versionFilesNames = parseFromBufferedReader(
List<String> versionRelativePaths = parseFromBufferedReader(
component,
"/transport/defined/manifest.txt",
"/transport/definitions/manifest.txt",
nameToStream,
(c, p, br) -> br.lines().filter(line -> line.isBlank() == false).toList()
);
if (versionFilesNames != null) {
Map<String, TransportVersion> transportVersions = new HashMap<>();
for (String versionFileName : versionFilesNames) {
if (versionRelativePaths != null) {
List<TransportVersion> transportVersions = new ArrayList<>();
for (String versionRelativePath : versionRelativePaths) {
TransportVersion transportVersion = parseFromBufferedReader(
component,
"/transport/defined/" + versionFileName,
"/transport/definitions/" + versionRelativePath,
nameToStream,
(c, p, br) -> fromBufferedReader(c, p, false, br, latest.id())
(c, p, br) -> fromBufferedReader(c, p, false, versionRelativePath.startsWith("initial/"), br, latest.id())
);
if (transportVersion != null) {
transportVersions.put(versionFileName.substring(0, versionFileName.length() - 4), transportVersion);
transportVersions.add(transportVersion);
}
}
return transportVersions;
}
}
return Map.of();
return List.of();
}

private static String toComponentPath(String component, String path) {
Expand Down Expand Up @@ -419,12 +426,15 @@ private static class VersionsHolder {
static {
// collect all the transport versions from server and es modules/plugins (defined in server)
List<TransportVersion> allVersions = new ArrayList<>(TransportVersions.DEFINED_VERSIONS);
Map<String, TransportVersion> allVersionsByName = collectFromInputStreams(
List<TransportVersion> streamVersions = collectFromInputStreams(
"<server>",
TransportVersion.class::getResourceAsStream,
Version.CURRENT.major + "." + Version.CURRENT.minor + ".csv"
);
addTransportVersions(allVersionsByName.values(), allVersions).sort(TransportVersion::compareTo);
Map<String, TransportVersion> allVersionsByName = streamVersions.stream()
.filter(tv -> tv.name() != null)
.collect(Collectors.toMap(TransportVersion::name, v -> v));
addTransportVersions(streamVersions, allVersions).sort(TransportVersion::compareTo);

// set version lookup by release before adding serverless versions
// serverless versions should not affect release version
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -228,9 +228,9 @@ public void testDuplicateConstants() {
public void testLatest() {
TransportVersion latest = TransportVersion.parseFromBufferedReader(
"<test>",
"/transport/defined/" + Version.CURRENT.major + "." + Version.CURRENT.minor + ".csv",
"/transport/definitions/" + Version.CURRENT.major + "." + Version.CURRENT.minor + ".csv",
TransportVersion.class::getResourceAsStream,
(c, p, br) -> TransportVersion.fromBufferedReader(c, p, true, br, Integer.MAX_VALUE)
(c, p, br) -> TransportVersion.fromBufferedReader(c, p, true, false, br, Integer.MAX_VALUE)
);
// TODO: once placeholder is removed, test the latest known version can be found fromName
// assertThat(latest, is(TransportVersion.fromName(latest.name())));
Expand All @@ -242,6 +242,7 @@ public void testSupports() {
"<test>",
"testSupports0",
false,
false,
new BufferedReader(new InputStreamReader(new ByteArrayInputStream(data0), StandardCharsets.UTF_8)),
5000000
);
Expand All @@ -254,6 +255,7 @@ public void testSupports() {
"<test>",
"testSupports1",
false,
false,
new BufferedReader(new InputStreamReader(new ByteArrayInputStream(data1), StandardCharsets.UTF_8)),
5000000
);
Expand All @@ -269,6 +271,7 @@ public void testSupports() {
"<test>",
"testSupports2",
false,
false,
new BufferedReader(new InputStreamReader(new ByteArrayInputStream(data2), StandardCharsets.UTF_8)),
5000000
);
Expand Down Expand Up @@ -296,6 +299,7 @@ public void testSupports() {
"<test>",
"testSupports3",
false,
false,
new BufferedReader(new InputStreamReader(new ByteArrayInputStream(data3), StandardCharsets.UTF_8)),
5000000
);
Expand Down Expand Up @@ -324,6 +328,7 @@ public void testSupports() {
"<test>",
"testSupports3",
false,
false,
new BufferedReader(new InputStreamReader(new ByteArrayInputStream(data4), StandardCharsets.UTF_8)),
5000000
);
Expand Down