Skip to content
Merged
Show file tree
Hide file tree
Changes from 7 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().replace('\\', '/');
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
58 changes: 42 additions & 16 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 @@ -228,7 +235,23 @@ public static TransportVersion fromId(int id) {
public static TransportVersion fromName(String name) {
TransportVersion known = VersionsHolder.ALL_VERSIONS_BY_NAME.get(name);
if (known == null) {
throw new IllegalStateException("unknown transport version [" + name + "]");
List<String> versionRelativePaths = parseFromBufferedReader(
"<server>",
"/transport/definitions/manifest.txt",
TransportVersion.class::getResourceAsStream,
(c, p, br) -> br.lines().filter(line -> line.isBlank() == false).toList()
);
throw new IllegalStateException(
"\nunknown transport version ["
+ name
+ "];"
+ "\nknown names ["
+ VersionsHolder.ALL_VERSIONS_BY_NAME
Copy link
Member

Choose a reason for hiding this comment

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

do we want this permanently or was it only for debugging?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought about keeping this at least until we are further along, but decided to remove it unless we need it again.

+ "]"
+ "\nrelative paths"
+ versionRelativePaths
+ "\n"
);
}
return known;
}
Expand Down Expand Up @@ -419,12 +442,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
Loading