From f8251b7ae12efbb403e2c99ca20188e88488c40f Mon Sep 17 00:00:00 2001 From: Ryan Ernst Date: Mon, 29 Sep 2025 16:10:28 -0700 Subject: [PATCH] Add back resetting transport upper bounds (#135322) When upper bounds files have been incorrectly modified, or changes to them are no longer needed, transport version generation should rebuild the upper bound. This commit adds back the ability to reset the upper bound files in these cases and adds scanning for the latest within a branch to find the appropriate id for a given upper bound file. --- .../TransportVersionGenerationFuncTest.groovy | 12 +--- ...enerateTransportVersionDefinitionTask.java | 62 ++++++++++++++----- .../TransportVersionResourcesService.java | 59 ++++++++++++++---- ...ValidateTransportVersionResourcesTask.java | 30 ++------- 4 files changed, 100 insertions(+), 63 deletions(-) diff --git a/build-tools-internal/src/integTest/groovy/org/elasticsearch/gradle/internal/transport/TransportVersionGenerationFuncTest.groovy b/build-tools-internal/src/integTest/groovy/org/elasticsearch/gradle/internal/transport/TransportVersionGenerationFuncTest.groovy index a5d7abed3c0ac..7dcdf5f66d166 100644 --- a/build-tools-internal/src/integTest/groovy/org/elasticsearch/gradle/internal/transport/TransportVersionGenerationFuncTest.groovy +++ b/build-tools-internal/src/integTest/groovy/org/elasticsearch/gradle/internal/transport/TransportVersionGenerationFuncTest.groovy @@ -90,9 +90,6 @@ class TransportVersionGenerationFuncTest extends AbstractTransportVersionFuncTes assertUpperBound("9.2", "new_tv,8124000") } - /* - temporarily muted, see https://github.com/elastic/elasticsearch/pull/135226 - def "invalid changes to a upper bounds should be reverted"() { given: transportVersionUpperBound("9.2", "modification", "9000000") @@ -147,7 +144,7 @@ class TransportVersionGenerationFuncTest extends AbstractTransportVersionFuncTes assertReferableDefinitionDoesNotExist("test_tv") assertUpperBound("9.2", "existing_92,8123000") assertUpperBound("9.1", "existing_92,8012001") - }*/ + } def "a reference can be renamed"() { given: @@ -245,11 +242,8 @@ class TransportVersionGenerationFuncTest extends AbstractTransportVersionFuncTes def "unreferenced definitions are removed"() { given: referableTransportVersion("test_tv", "8124000,8012002") - /* - TODO: reset of upper bounds transportVersionUpperBound("9.2", "test_tv", "8124000") transportVersionUpperBound("9.1", "test_tv", "8012002") - */ when: def result = runGenerateAndValidateTask().build() @@ -412,8 +406,6 @@ class TransportVersionGenerationFuncTest extends AbstractTransportVersionFuncTes assertUpperBound("9.2", "new_tv,8124000") } - /* - TODO: reset of upper bounds def "deleted upper bounds files are restored"() { given: file("myserver/src/main/resources/transport/upper_bounds/9.2.csv").delete() @@ -424,7 +416,7 @@ class TransportVersionGenerationFuncTest extends AbstractTransportVersionFuncTes then: assertGenerateAndValidateSuccess(result) assertUpperBound("9.2", "existing_92,8123000") - }*/ + } def "upper bounds files must exist for backport branches"() { when: diff --git a/build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/transport/GenerateTransportVersionDefinitionTask.java b/build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/transport/GenerateTransportVersionDefinitionTask.java index d138ea717266e..4ba262ee16a10 100644 --- a/build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/transport/GenerateTransportVersionDefinitionTask.java +++ b/build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/transport/GenerateTransportVersionDefinitionTask.java @@ -9,6 +9,7 @@ package org.elasticsearch.gradle.internal.transport; +import org.elasticsearch.gradle.internal.transport.TransportVersionResourcesService.IdAndDefinition; import org.gradle.api.DefaultTask; import org.gradle.api.file.ConfigurableFileCollection; import org.gradle.api.file.RegularFileProperty; @@ -31,6 +32,7 @@ import java.util.Collections; import java.util.HashSet; import java.util.List; +import java.util.Map; import java.util.Set; /** @@ -95,29 +97,39 @@ public abstract class GenerateTransportVersionDefinitionTask extends DefaultTask public void run() throws IOException { TransportVersionResourcesService resources = getResourceService().get(); Set referencedNames = TransportVersionReference.collectNames(getReferencesFiles()); - List changedDefinitionNames = resources.getChangedReferableDefinitionNames(); + Set changedDefinitionNames = resources.getChangedReferableDefinitionNames(); String targetDefinitionName = getTargetDefinitionName(resources, referencedNames, changedDefinitionNames); - getLogger().lifecycle("Generating transport version name: " + targetDefinitionName); + // First check for any unused definitions. This later generation to not get confused by a definition that can't be used. + removeUnusedNamedDefinitions(resources, referencedNames, changedDefinitionNames); + + Map> idsByBase = resources.getIdsByBase(); if (targetDefinitionName.isEmpty()) { - // TODO: resetting upper bounds needs to be done locally, otherwise it pulls in some (incomplete) changes from upstream main - // resetAllUpperBounds(resources); + getLogger().lifecycle("No transport version name detected, resetting upper bounds"); + resetAllUpperBounds(resources, idsByBase); } else { + getLogger().lifecycle("Generating transport version name: " + targetDefinitionName); List upstreamUpperBounds = resources.getUpperBoundsFromUpstream(); Set targetUpperBoundNames = getTargetUpperBoundNames(resources, upstreamUpperBounds, targetDefinitionName); - List ids = updateUpperBounds(resources, upstreamUpperBounds, targetUpperBoundNames, targetDefinitionName); + List ids = updateUpperBounds( + resources, + upstreamUpperBounds, + targetUpperBoundNames, + idsByBase, + targetDefinitionName + ); // (Re)write the definition file. resources.writeDefinition(new TransportVersionDefinition(targetDefinitionName, ids, true)); } - removeUnusedNamedDefinitions(resources, referencedNames, changedDefinitionNames); } private List updateUpperBounds( TransportVersionResourcesService resources, List existingUpperBounds, Set targetUpperBoundNames, + Map> idsByBase, String definitionName ) throws IOException { String currentUpperBoundName = getCurrentUpperBoundName().get(); @@ -146,9 +158,9 @@ private List updateUpperBounds( resources.writeUpperBound(newUpperBound, stageInGit); } ids.add(targetId); - } else { + } else if (resources.getChangedUpperBoundNames().contains(upperBoundName)) { // Default case: we're not targeting this branch so reset it - resources.writeUpperBound(existingUpperBound, false); + resetUpperBound(resources, existingUpperBound, idsByBase, definitionName); } } @@ -159,7 +171,7 @@ private List updateUpperBounds( private String getTargetDefinitionName( TransportVersionResourcesService resources, Set referencedNames, - List changedDefinitions + Set changedDefinitions ) { if (getDefinitionName().isPresent()) { // an explicit name was passed in, so use it @@ -180,7 +192,7 @@ private String getTargetDefinitionName( if (changedDefinitions.isEmpty()) { return ""; } else { - String changedDefinitionName = changedDefinitions.getFirst(); + String changedDefinitionName = changedDefinitions.iterator().next(); if (referencedNames.contains(changedDefinitionName)) { return changedDefinitionName; } else { @@ -248,16 +260,38 @@ private Set getUpperBoundNamesFromDefinition( return upperBoundNames; } - private void resetAllUpperBounds(TransportVersionResourcesService resources) throws IOException { - for (TransportVersionUpperBound upperBound : resources.getUpperBoundsFromUpstream()) { - resources.writeUpperBound(upperBound, false); + private void resetAllUpperBounds(TransportVersionResourcesService resources, Map> idsByBase) + throws IOException { + for (String upperBoundName : resources.getChangedUpperBoundNames()) { + TransportVersionUpperBound upstreamUpperBound = resources.getUpperBoundFromUpstream(upperBoundName); + resetUpperBound(resources, upstreamUpperBound, idsByBase, null); + } + } + + private void resetUpperBound( + TransportVersionResourcesService resources, + TransportVersionUpperBound upperBound, + Map> idsByBase, + String ignoreDefinitionName + ) throws IOException { + List idsForUpperBound = idsByBase.get(upperBound.definitionId().base()); + if (idsForUpperBound == null) { + throw new RuntimeException("Could not find base id: " + upperBound.definitionId().base()); + } + IdAndDefinition resetValue = idsForUpperBound.getLast(); + if (resetValue.definition().name().equals(ignoreDefinitionName)) { + // there must be another definition in this base since the ignored definition is new + assert idsForUpperBound.size() >= 2; + resetValue = idsForUpperBound.get(idsForUpperBound.size() - 2); } + var resetUpperBound = new TransportVersionUpperBound(upperBound.name(), resetValue.definition().name(), resetValue.id()); + resources.writeUpperBound(resetUpperBound, false); } private void removeUnusedNamedDefinitions( TransportVersionResourcesService resources, Set referencedNames, - List changedDefinitions + Set changedDefinitions ) throws IOException { for (String definitionName : changedDefinitions) { if (referencedNames.contains(definitionName) == false) { diff --git a/build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/transport/TransportVersionResourcesService.java b/build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/transport/TransportVersionResourcesService.java index 251f514002da6..5be53829d06f8 100644 --- a/build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/transport/TransportVersionResourcesService.java +++ b/build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/transport/TransportVersionResourcesService.java @@ -26,6 +26,7 @@ import java.nio.file.Path; import java.util.ArrayList; import java.util.Collections; +import java.util.Comparator; import java.util.HashMap; import java.util.HashSet; import java.util.List; @@ -33,6 +34,7 @@ import java.util.Set; import java.util.concurrent.atomic.AtomicReference; import java.util.function.BiFunction; +import java.util.function.Consumer; import java.util.stream.Collectors; import javax.inject.Inject; @@ -66,6 +68,8 @@ public interface Parameters extends BuildServiceParameters { Property getUpstreamRefOverride(); } + record IdAndDefinition(TransportVersionId id, TransportVersionDefinition definition) {} + @Inject public abstract ExecOperations getExecOperations(); @@ -129,19 +133,8 @@ TransportVersionDefinition getReferableDefinitionFromUpstream(String name) { } /** Get the definition names which have local changes relative to upstream */ - List getChangedReferableDefinitionNames() { - List changedDefinitions = new ArrayList<>(); - // make sure the prefix is git style paths, always forward slashes - String referablePrefix = REFERABLE_DIR.toString().replace('\\', '/'); - for (String changedPath : getChangedResources()) { - if (changedPath.contains(referablePrefix) == false) { - continue; - } - int lastSlashNdx = changedPath.lastIndexOf('/'); - String name = changedPath.substring(lastSlashNdx + 1, changedPath.length() - 4 /* .csv */); - changedDefinitions.add(name); - } - return changedDefinitions; + Set getChangedReferableDefinitionNames() { + return getChangedNames(REFERABLE_DIR); } /** Test whether the given referable definition exists */ @@ -187,6 +180,27 @@ TransportVersionDefinition getUnreferableDefinitionFromUpstream(String name) { return getUpstreamFile(resourcePath, (path, contents) -> TransportVersionDefinition.fromString(path, contents, false)); } + /** Get all the ids and definitions for a given base id, sorted by the complete id */ + Map> getIdsByBase() throws IOException { + Map> idsByBase = new HashMap<>(); + + // first collect all ids, organized by base + Consumer addToBase = definition -> { + for (TransportVersionId id : definition.ids()) { + idsByBase.computeIfAbsent(id.base(), k -> new ArrayList<>()).add(new IdAndDefinition(id, definition)); + } + }; + getReferableDefinitions().values().forEach(addToBase); + getUnreferableDefinitions().values().forEach(addToBase); + + // now sort the ids within each base so we can check density later + for (var ids : idsByBase.values()) { + // first sort the ids list so we can check compactness and quickly lookup the highest id later + ids.sort(Comparator.comparingInt(a -> a.id().complete())); + } + return idsByBase; + } + /** Read all upper bound files and return them mapped by their release name */ Map getUpperBounds() throws IOException { Map upperBounds = new HashMap<>(); @@ -220,6 +234,10 @@ List getUpperBoundsFromUpstream() throws IOException return upperBounds; } + Set getChangedUpperBoundNames() { + return getChangedNames(UPPER_BOUNDS_DIR); + } + /** Write the given upper bound to a file in the transport resources */ void writeUpperBound(TransportVersionUpperBound upperBound, boolean stageInGit) throws IOException { Path path = transportResourcesDir.resolve(getUpperBoundRelativePath(upperBound.name())); @@ -317,6 +335,21 @@ private Set getChangedResources() { return changedResources.get(); } + private Set getChangedNames(Path resourcesDir) { + Set changedNames = new HashSet<>(); + // make sure the prefix is git style paths, always forward slashes + String resourcesPrefix = resourcesDir.toString().replace('\\', '/'); + for (String changedPath : getChangedResources()) { + if (changedPath.contains(resourcesPrefix) == false) { + continue; + } + int lastSlashNdx = changedPath.lastIndexOf('/'); + String name = changedPath.substring(lastSlashNdx + 1, changedPath.length() - 4 /* .csv */); + changedNames.add(name); + } + return changedNames; + } + // Read a transport version resource from the upstream, or return null if it doesn't exist there private T getUpstreamFile(Path resourcePath, BiFunction parser) { String pathString = resourcePath.toString().replace('\\', '/'); // normalize to forward slash that git uses diff --git a/build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/transport/ValidateTransportVersionResourcesTask.java b/build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/transport/ValidateTransportVersionResourcesTask.java index 8a69158ff4e07..02338d7f677f1 100644 --- a/build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/transport/ValidateTransportVersionResourcesTask.java +++ b/build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/transport/ValidateTransportVersionResourcesTask.java @@ -11,6 +11,7 @@ import com.google.common.collect.Comparators; +import org.elasticsearch.gradle.internal.transport.TransportVersionResourcesService.IdAndDefinition; import org.gradle.api.DefaultTask; import org.gradle.api.file.ConfigurableFileCollection; import org.gradle.api.provider.Property; @@ -28,8 +29,6 @@ import java.io.IOException; import java.nio.file.Path; -import java.util.ArrayList; -import java.util.Collection; import java.util.Comparator; import java.util.HashMap; import java.util.List; @@ -67,8 +66,6 @@ public Path getResourcesDir() { @Input public abstract Property getCurrentUpperBoundName(); - private record IdAndDefinition(TransportVersionId id, TransportVersionDefinition definition) {} - private static final Pattern NAME_FORMAT = Pattern.compile("[a-z0-9_]+"); @ServiceReference("transportVersionResources") @@ -81,7 +78,7 @@ public void validateTransportVersions() throws IOException { Map referableDefinitions = resources.getReferableDefinitions(); Map unreferableDefinitions = resources.getUnreferableDefinitions(); Map allDefinitions = collectAllDefinitions(referableDefinitions, unreferableDefinitions); - Map> idsByBase = collectIdsByBase(allDefinitions.values()); + Map> idsByBase = resources.getIdsByBase(); Map upperBounds = resources.getUpperBounds(); TransportVersionUpperBound currentUpperBound = upperBounds.get(getCurrentUpperBoundName().get()); boolean onReleaseBranch = checkIfDefinitelyOnReleaseBranch(upperBounds); @@ -129,25 +126,6 @@ private Map collectAllDefinitions( return allDefinitions; } - private Map> collectIdsByBase(Collection definitions) { - Map> idsByBase = new HashMap<>(); - - // first collect all ids, organized by base - for (TransportVersionDefinition definition : definitions) { - for (TransportVersionId id : definition.ids()) { - idsByBase.computeIfAbsent(id.base(), k -> new ArrayList<>()).add(new IdAndDefinition(id, definition)); - } - } - - // now sort the ids within each base so we can check density later - for (var ids : idsByBase.values()) { - // first sort the ids list so we can check compactness and quickly lookup the highest id later - ids.sort(Comparator.comparingInt(a -> a.id().complete())); - } - - return idsByBase; - } - private void validateNamedDefinition(TransportVersionDefinition definition, Set referencedNames) { if (referencedNames.contains(definition.name()) == false) { throwDefinitionFailure(definition, "is not referenced"); @@ -280,10 +258,10 @@ private void validateBase(int base, List ids) { IdAndDefinition current = ids.get(ndx); if (previous.id().equals(current.id())) { - Path existingDefinitionPath = getResources().get().getDefinitionPath(previous.definition); + Path existingDefinitionPath = getResources().get().getDefinitionPath(previous.definition()); throwDefinitionFailure( current.definition(), - "contains id " + current.id + " already defined in [" + existingDefinitionPath + "]" + "contains id " + current.id() + " already defined in [" + existingDefinitionPath + "]" ); }