Skip to content

Commit c6dec22

Browse files
rjernstrjernst
authored
Add back resetting transport upper bounds (elastic#135322) (elastic#135664)
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.
1 parent 42e2b66 commit c6dec22

File tree

4 files changed

+103
-66
lines changed

4 files changed

+103
-66
lines changed

‎build-tools-internal/src/integTest/groovy/org/elasticsearch/gradle/internal/transport/TransportVersionGenerationFuncTest.groovy‎

Lines changed: 3 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -90,9 +90,6 @@ class TransportVersionGenerationFuncTest extends AbstractTransportVersionFuncTes
9090
assertUpperBound("9.2", "new_tv,8124000")
9191
}
9292

93-
/*
94-
temporarily muted, see https://github.com/elastic/elasticsearch/pull/135226
95-
9693
def "invalid changes to a upper bounds should be reverted"() {
9794
given:
9895
transportVersionUpperBound("9.2", "modification", "9000000")
@@ -147,7 +144,7 @@ class TransportVersionGenerationFuncTest extends AbstractTransportVersionFuncTes
147144
assertReferableDefinitionDoesNotExist("test_tv")
148145
assertUpperBound("9.2", "existing_92,8123000")
149146
assertUpperBound("9.1", "existing_92,8012001")
150-
}*/
147+
}
151148

152149
def "a reference can be renamed"() {
153150
given:
@@ -245,11 +242,8 @@ class TransportVersionGenerationFuncTest extends AbstractTransportVersionFuncTes
245242
def "unreferenced definitions are removed"() {
246243
given:
247244
referableTransportVersion("test_tv", "8124000,8012002")
248-
/*
249-
TODO: reset of upper bounds
250245
transportVersionUpperBound("9.2", "test_tv", "8124000")
251246
transportVersionUpperBound("9.1", "test_tv", "8012002")
252-
*/
253247

254248
when:
255249
def result = runGenerateAndValidateTask().build()
@@ -412,8 +406,6 @@ class TransportVersionGenerationFuncTest extends AbstractTransportVersionFuncTes
412406
assertUpperBound("9.2", "new_tv,8124000")
413407
}
414408

415-
/*
416-
TODO: reset of upper bounds
417409
def "deleted upper bounds files are restored"() {
418410
given:
419411
file("myserver/src/main/resources/transport/upper_bounds/9.2.csv").delete()
@@ -424,11 +416,11 @@ class TransportVersionGenerationFuncTest extends AbstractTransportVersionFuncTes
424416
then:
425417
assertGenerateAndValidateSuccess(result)
426418
assertUpperBound("9.2", "existing_92,8123000")
427-
}*/
419+
}
428420

429421
def "upper bounds files must exist for backport branches"() {
430422
when:
431-
def result = runGenerateTask("--backport-branches=9.1,8.13,7.17,6.0").buildAndFail()
423+
def result = runGenerateTask("--name", "new_tv", "--backport-branches=9.1,8.13,7.17,6.0").buildAndFail()
432424

433425
then:
434426
assertGenerateFailure(result, "Missing upper bounds files for branches [6.0, 7.17, 8.13], known branches are [8.19, 9.0, 9.1, 9.2]")

‎build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/transport/GenerateTransportVersionDefinitionTask.java‎

Lines changed: 50 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99

1010
package org.elasticsearch.gradle.internal.transport;
1111

12+
import org.elasticsearch.gradle.internal.transport.TransportVersionResourcesService.IdAndDefinition;
1213
import org.gradle.api.DefaultTask;
1314
import org.gradle.api.file.ConfigurableFileCollection;
1415
import org.gradle.api.file.RegularFileProperty;
@@ -31,6 +32,7 @@
3132
import java.util.Collections;
3233
import java.util.HashSet;
3334
import java.util.List;
35+
import java.util.Map;
3436
import java.util.Set;
3537

3638
/**
@@ -95,29 +97,39 @@ public abstract class GenerateTransportVersionDefinitionTask extends DefaultTask
9597
public void run() throws IOException {
9698
TransportVersionResourcesService resources = getResourceService().get();
9799
Set<String> referencedNames = TransportVersionReference.collectNames(getReferencesFiles());
98-
List<String> changedDefinitionNames = resources.getChangedReferableDefinitionNames();
100+
Set<String> changedDefinitionNames = resources.getChangedReferableDefinitionNames();
99101
String targetDefinitionName = getTargetDefinitionName(resources, referencedNames, changedDefinitionNames);
100102

101-
List<TransportVersionUpperBound> upstreamUpperBounds = resources.getUpperBoundsFromUpstream();
102-
Set<String> targetUpperBoundNames = getTargetUpperBoundNames(resources, upstreamUpperBounds, targetDefinitionName);
103+
// First check for any unused definitions. This later generation to not get confused by a definition that can't be used.
104+
removeUnusedNamedDefinitions(resources, referencedNames, changedDefinitionNames);
103105

104-
getLogger().lifecycle("Generating transport version name: " + targetDefinitionName);
106+
Map<Integer, List<IdAndDefinition>> idsByBase = resources.getIdsByBase();
105107
if (targetDefinitionName.isEmpty()) {
106-
// TODO: resetting upper bounds needs to be done locally, otherwise it pulls in some (incomplete) changes from upstream main
107-
// resetAllUpperBounds(resources);
108+
getLogger().lifecycle("No transport version name detected, resetting upper bounds");
109+
resetAllUpperBounds(resources, idsByBase);
108110
} else {
109-
List<TransportVersionId> ids = updateUpperBounds(resources, upstreamUpperBounds, targetUpperBoundNames, targetDefinitionName);
111+
getLogger().lifecycle("Generating transport version name: " + targetDefinitionName);
112+
List<TransportVersionUpperBound> upstreamUpperBounds = resources.getUpperBoundsFromUpstream();
113+
Set<String> targetUpperBoundNames = getTargetUpperBoundNames(resources, upstreamUpperBounds, targetDefinitionName);
114+
115+
List<TransportVersionId> ids = updateUpperBounds(
116+
resources,
117+
upstreamUpperBounds,
118+
targetUpperBoundNames,
119+
idsByBase,
120+
targetDefinitionName
121+
);
110122
// (Re)write the definition file.
111123
resources.writeDefinition(new TransportVersionDefinition(targetDefinitionName, ids, true));
112124
}
113125

114-
removeUnusedNamedDefinitions(resources, referencedNames, changedDefinitionNames);
115126
}
116127

117128
private List<TransportVersionId> updateUpperBounds(
118129
TransportVersionResourcesService resources,
119130
List<TransportVersionUpperBound> existingUpperBounds,
120131
Set<String> targetUpperBoundNames,
132+
Map<Integer, List<IdAndDefinition>> idsByBase,
121133
String definitionName
122134
) throws IOException {
123135
String currentUpperBoundName = getCurrentUpperBoundName().get();
@@ -146,9 +158,9 @@ private List<TransportVersionId> updateUpperBounds(
146158
resources.writeUpperBound(newUpperBound, stageInGit);
147159
}
148160
ids.add(targetId);
149-
} else {
161+
} else if (resources.getChangedUpperBoundNames().contains(upperBoundName)) {
150162
// Default case: we're not targeting this branch so reset it
151-
resources.writeUpperBound(existingUpperBound, false);
163+
resetUpperBound(resources, existingUpperBound, idsByBase, definitionName);
152164
}
153165
}
154166

@@ -159,7 +171,7 @@ private List<TransportVersionId> updateUpperBounds(
159171
private String getTargetDefinitionName(
160172
TransportVersionResourcesService resources,
161173
Set<String> referencedNames,
162-
List<String> changedDefinitions
174+
Set<String> changedDefinitions
163175
) {
164176
if (getDefinitionName().isPresent()) {
165177
// an explicit name was passed in, so use it
@@ -180,7 +192,7 @@ private String getTargetDefinitionName(
180192
if (changedDefinitions.isEmpty()) {
181193
return "";
182194
} else {
183-
String changedDefinitionName = changedDefinitions.get(0);
195+
String changedDefinitionName = changedDefinitions.iterator().next();
184196
if (referencedNames.contains(changedDefinitionName)) {
185197
return changedDefinitionName;
186198
} else {
@@ -248,16 +260,38 @@ private Set<String> getUpperBoundNamesFromDefinition(
248260
return upperBoundNames;
249261
}
250262

251-
private void resetAllUpperBounds(TransportVersionResourcesService resources) throws IOException {
252-
for (TransportVersionUpperBound upperBound : resources.getUpperBoundsFromUpstream()) {
253-
resources.writeUpperBound(upperBound, false);
263+
private void resetAllUpperBounds(TransportVersionResourcesService resources, Map<Integer, List<IdAndDefinition>> idsByBase)
264+
throws IOException {
265+
for (String upperBoundName : resources.getChangedUpperBoundNames()) {
266+
TransportVersionUpperBound upstreamUpperBound = resources.getUpperBoundFromUpstream(upperBoundName);
267+
resetUpperBound(resources, upstreamUpperBound, idsByBase, null);
268+
}
269+
}
270+
271+
private void resetUpperBound(
272+
TransportVersionResourcesService resources,
273+
TransportVersionUpperBound upperBound,
274+
Map<Integer, List<IdAndDefinition>> idsByBase,
275+
String ignoreDefinitionName
276+
) throws IOException {
277+
List<IdAndDefinition> idsForUpperBound = idsByBase.get(upperBound.definitionId().base());
278+
if (idsForUpperBound == null) {
279+
throw new RuntimeException("Could not find base id: " + upperBound.definitionId().base());
280+
}
281+
IdAndDefinition resetValue = idsForUpperBound.get(idsForUpperBound.size() - 1);
282+
if (resetValue.definition().name().equals(ignoreDefinitionName)) {
283+
// there must be another definition in this base since the ignored definition is new
284+
assert idsForUpperBound.size() >= 2;
285+
resetValue = idsForUpperBound.get(idsForUpperBound.size() - 2);
254286
}
287+
var resetUpperBound = new TransportVersionUpperBound(upperBound.name(), resetValue.definition().name(), resetValue.id());
288+
resources.writeUpperBound(resetUpperBound, false);
255289
}
256290

257291
private void removeUnusedNamedDefinitions(
258292
TransportVersionResourcesService resources,
259293
Set<String> referencedNames,
260-
List<String> changedDefinitions
294+
Set<String> changedDefinitions
261295
) throws IOException {
262296
for (String definitionName : changedDefinitions) {
263297
if (referencedNames.contains(definitionName) == false) {

‎build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/transport/TransportVersionResourcesService.java‎

Lines changed: 46 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -26,13 +26,15 @@
2626
import java.nio.file.Path;
2727
import java.util.ArrayList;
2828
import java.util.Collections;
29+
import java.util.Comparator;
2930
import java.util.HashMap;
3031
import java.util.HashSet;
3132
import java.util.List;
3233
import java.util.Map;
3334
import java.util.Set;
3435
import java.util.concurrent.atomic.AtomicReference;
3536
import java.util.function.BiFunction;
37+
import java.util.function.Consumer;
3638
import java.util.stream.Collectors;
3739

3840
import javax.inject.Inject;
@@ -66,6 +68,8 @@ public interface Parameters extends BuildServiceParameters {
6668
Property<String> getUpstreamRefOverride();
6769
}
6870

71+
record IdAndDefinition(TransportVersionId id, TransportVersionDefinition definition) {}
72+
6973
@Inject
7074
public abstract ExecOperations getExecOperations();
7175

@@ -129,19 +133,8 @@ TransportVersionDefinition getReferableDefinitionFromUpstream(String name) {
129133
}
130134

131135
/** Get the definition names which have local changes relative to upstream */
132-
List<String> getChangedReferableDefinitionNames() {
133-
List<String> changedDefinitions = new ArrayList<>();
134-
// make sure the prefix is git style paths, always forward slashes
135-
String referablePrefix = REFERABLE_DIR.toString().replace('\\', '/');
136-
for (String changedPath : getChangedResources()) {
137-
if (changedPath.contains(referablePrefix) == false) {
138-
continue;
139-
}
140-
int lastSlashNdx = changedPath.lastIndexOf('/');
141-
String name = changedPath.substring(lastSlashNdx + 1, changedPath.length() - 4 /* .csv */);
142-
changedDefinitions.add(name);
143-
}
144-
return changedDefinitions;
136+
Set<String> getChangedReferableDefinitionNames() {
137+
return getChangedNames(REFERABLE_DIR);
145138
}
146139

147140
/** Test whether the given referable definition exists */
@@ -187,6 +180,27 @@ TransportVersionDefinition getUnreferableDefinitionFromUpstream(String name) {
187180
return getUpstreamFile(resourcePath, (path, contents) -> TransportVersionDefinition.fromString(path, contents, false));
188181
}
189182

183+
/** Get all the ids and definitions for a given base id, sorted by the complete id */
184+
Map<Integer, List<IdAndDefinition>> getIdsByBase() throws IOException {
185+
Map<Integer, List<IdAndDefinition>> idsByBase = new HashMap<>();
186+
187+
// first collect all ids, organized by base
188+
Consumer<TransportVersionDefinition> addToBase = definition -> {
189+
for (TransportVersionId id : definition.ids()) {
190+
idsByBase.computeIfAbsent(id.base(), k -> new ArrayList<>()).add(new IdAndDefinition(id, definition));
191+
}
192+
};
193+
getReferableDefinitions().values().forEach(addToBase);
194+
getUnreferableDefinitions().values().forEach(addToBase);
195+
196+
// now sort the ids within each base so we can check density later
197+
for (var ids : idsByBase.values()) {
198+
// first sort the ids list so we can check compactness and quickly lookup the highest id later
199+
ids.sort(Comparator.comparingInt(a -> a.id().complete()));
200+
}
201+
return idsByBase;
202+
}
203+
190204
/** Read all upper bound files and return them mapped by their release name */
191205
Map<String, TransportVersionUpperBound> getUpperBounds() throws IOException {
192206
Map<String, TransportVersionUpperBound> upperBounds = new HashMap<>();
@@ -220,6 +234,10 @@ List<TransportVersionUpperBound> getUpperBoundsFromUpstream() throws IOException
220234
return upperBounds;
221235
}
222236

237+
Set<String> getChangedUpperBoundNames() {
238+
return getChangedNames(UPPER_BOUNDS_DIR);
239+
}
240+
223241
/** Write the given upper bound to a file in the transport resources */
224242
void writeUpperBound(TransportVersionUpperBound upperBound, boolean stageInGit) throws IOException {
225243
Path path = transportResourcesDir.resolve(getUpperBoundRelativePath(upperBound.name()));
@@ -317,6 +335,21 @@ private Set<String> getChangedResources() {
317335
return changedResources.get();
318336
}
319337

338+
private Set<String> getChangedNames(Path resourcesDir) {
339+
Set<String> changedNames = new HashSet<>();
340+
// make sure the prefix is git style paths, always forward slashes
341+
String resourcesPrefix = resourcesDir.toString().replace('\\', '/');
342+
for (String changedPath : getChangedResources()) {
343+
if (changedPath.contains(resourcesPrefix) == false) {
344+
continue;
345+
}
346+
int lastSlashNdx = changedPath.lastIndexOf('/');
347+
String name = changedPath.substring(lastSlashNdx + 1, changedPath.length() - 4 /* .csv */);
348+
changedNames.add(name);
349+
}
350+
return changedNames;
351+
}
352+
320353
// Read a transport version resource from the upstream, or return null if it doesn't exist there
321354
private <T> T getUpstreamFile(Path resourcePath, BiFunction<Path, String, T> parser) {
322355
String pathString = resourcePath.toString().replace('\\', '/'); // normalize to forward slash that git uses

‎build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/transport/ValidateTransportVersionResourcesTask.java‎

Lines changed: 4 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111

1212
import com.google.common.collect.Comparators;
1313

14+
import org.elasticsearch.gradle.internal.transport.TransportVersionResourcesService.IdAndDefinition;
1415
import org.gradle.api.DefaultTask;
1516
import org.gradle.api.file.ConfigurableFileCollection;
1617
import org.gradle.api.provider.Property;
@@ -28,8 +29,6 @@
2829

2930
import java.io.IOException;
3031
import java.nio.file.Path;
31-
import java.util.ArrayList;
32-
import java.util.Collection;
3332
import java.util.Comparator;
3433
import java.util.HashMap;
3534
import java.util.List;
@@ -67,8 +66,6 @@ public Path getResourcesDir() {
6766
@Input
6867
public abstract Property<String> getCurrentUpperBoundName();
6968

70-
private record IdAndDefinition(TransportVersionId id, TransportVersionDefinition definition) {}
71-
7269
private static final Pattern NAME_FORMAT = Pattern.compile("[a-z0-9_]+");
7370

7471
@ServiceReference("transportVersionResources")
@@ -81,7 +78,7 @@ public void validateTransportVersions() throws IOException {
8178
Map<String, TransportVersionDefinition> referableDefinitions = resources.getReferableDefinitions();
8279
Map<String, TransportVersionDefinition> unreferableDefinitions = resources.getUnreferableDefinitions();
8380
Map<String, TransportVersionDefinition> allDefinitions = collectAllDefinitions(referableDefinitions, unreferableDefinitions);
84-
Map<Integer, List<IdAndDefinition>> idsByBase = collectIdsByBase(allDefinitions.values());
81+
Map<Integer, List<IdAndDefinition>> idsByBase = resources.getIdsByBase();
8582
Map<String, TransportVersionUpperBound> upperBounds = resources.getUpperBounds();
8683
TransportVersionUpperBound currentUpperBound = upperBounds.get(getCurrentUpperBoundName().get());
8784
boolean onReleaseBranch = checkIfDefinitelyOnReleaseBranch(upperBounds);
@@ -129,25 +126,6 @@ private Map<String, TransportVersionDefinition> collectAllDefinitions(
129126
return allDefinitions;
130127
}
131128

132-
private Map<Integer, List<IdAndDefinition>> collectIdsByBase(Collection<TransportVersionDefinition> definitions) {
133-
Map<Integer, List<IdAndDefinition>> idsByBase = new HashMap<>();
134-
135-
// first collect all ids, organized by base
136-
for (TransportVersionDefinition definition : definitions) {
137-
for (TransportVersionId id : definition.ids()) {
138-
idsByBase.computeIfAbsent(id.base(), k -> new ArrayList<>()).add(new IdAndDefinition(id, definition));
139-
}
140-
}
141-
142-
// now sort the ids within each base so we can check density later
143-
for (var ids : idsByBase.values()) {
144-
// first sort the ids list so we can check compactness and quickly lookup the highest id later
145-
ids.sort(Comparator.comparingInt(a -> a.id().complete()));
146-
}
147-
148-
return idsByBase;
149-
}
150-
151129
private void validateNamedDefinition(TransportVersionDefinition definition, Set<String> referencedNames) {
152130
if (referencedNames.contains(definition.name()) == false) {
153131
throwDefinitionFailure(definition, "is not referenced");
@@ -280,10 +258,10 @@ private void validateBase(int base, List<IdAndDefinition> ids) {
280258
IdAndDefinition current = ids.get(ndx);
281259

282260
if (previous.id().equals(current.id())) {
283-
Path existingDefinitionPath = getResources().get().getDefinitionPath(previous.definition);
261+
Path existingDefinitionPath = getResources().get().getDefinitionPath(previous.definition());
284262
throwDefinitionFailure(
285263
current.definition(),
286-
"contains id " + current.id + " already defined in [" + existingDefinitionPath + "]"
264+
"contains id " + current.id() + " already defined in [" + existingDefinitionPath + "]"
287265
);
288266
}
289267

0 commit comments

Comments
 (0)