Skip to content

Commit 64518fd

Browse files
jdconradjdconrad
authored
Merge branch 'main' into mtv24
2 parents 6a543e3 + d7b7666 commit 64518fd

File tree

4 files changed

+100
-63
lines changed

4 files changed

+100
-63
lines changed

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

Lines changed: 2 additions & 10 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,7 +416,7 @@ 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:

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

Lines changed: 48 additions & 14 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-
getLogger().lifecycle("Generating transport version name: " + 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);
105+
106+
Map<Integer, List<IdAndDefinition>> idsByBase = resources.getIdsByBase();
102107
if (targetDefinitionName.isEmpty()) {
103-
// TODO: resetting upper bounds needs to be done locally, otherwise it pulls in some (incomplete) changes from upstream main
104-
// resetAllUpperBounds(resources);
108+
getLogger().lifecycle("No transport version name detected, resetting upper bounds");
109+
resetAllUpperBounds(resources, idsByBase);
105110
} else {
111+
getLogger().lifecycle("Generating transport version name: " + targetDefinitionName);
106112
List<TransportVersionUpperBound> upstreamUpperBounds = resources.getUpperBoundsFromUpstream();
107113
Set<String> targetUpperBoundNames = getTargetUpperBoundNames(resources, upstreamUpperBounds, targetDefinitionName);
108114

109-
List<TransportVersionId> ids = updateUpperBounds(resources, upstreamUpperBounds, targetUpperBoundNames, targetDefinitionName);
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.getFirst();
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.getLast();
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)