-
Notifications
You must be signed in to change notification settings - Fork 0
Filter spawn strategies by execution platform #13
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from 11 commits
b166287
9c1e9ff
4da7fbb
b5f6348
3d17888
d02829e
0c32764
c7ca9b1
fca4828
88395ce
01357a0
c4eb49a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -76,7 +76,11 @@ public static Platform getPlatformProto( | |
| ? remoteOptions.getRemoteDefaultExecProperties() | ||
| : ImmutableSortedMap.of(); | ||
|
|
||
| if (spawn.getExecutionPlatform() == null | ||
| if ((spawn.getExecutionPlatform() == null | ||
| || ( | ||
| spawn.getExecutionPlatform().execProperties().isEmpty() | ||
| && spawn.getExecutionPlatform().remoteExecutionProperties().isEmpty() | ||
| )) | ||
arvi18 marked this conversation as resolved.
Show resolved
Hide resolved
arvi18 marked this conversation as resolved.
Show resolved
Hide resolved
arvi18 marked this conversation as resolved.
Show resolved
Hide resolved
arvi18 marked this conversation as resolved.
Show resolved
Hide resolved
arvi18 marked this conversation as resolved.
Show resolved
Hide resolved
arvi18 marked this conversation as resolved.
Show resolved
Hide resolved
arvi18 marked this conversation as resolved.
Show resolved
Hide resolved
arvi18 marked this conversation as resolved.
Show resolved
Hide resolved
arvi18 marked this conversation as resolved.
Show resolved
Hide resolved
arvi18 marked this conversation as resolved.
Show resolved
Hide resolved
arvi18 marked this conversation as resolved.
Show resolved
Hide resolved
Comment on lines
+80
to
+84
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Platform Condition LogicComplex conditional logic checks platform nullability then accesses platform methods without null safety. If spawn.getExecutionPlatform() returns null, subsequent calls to execProperties() and remoteExecutionProperties() will cause NullPointerException. Logic should use null-safe evaluation pattern. Standards
|
||
| && spawn.getCombinedExecProperties().isEmpty() | ||
| && defaultExecProperties.isEmpty() | ||
| && additionalProperties.isEmpty()) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,6 +18,7 @@ | |
| import com.google.devtools.build.lib.analysis.actions.FileWriteActionContext; | ||
| import com.google.devtools.build.lib.analysis.actions.TemplateExpansionContext; | ||
| import com.google.devtools.build.lib.buildtool.BuildRequest; | ||
| import com.google.devtools.build.lib.cmdline.Label; | ||
| import com.google.devtools.build.lib.exec.ExecutionOptions; | ||
| import com.google.devtools.build.lib.exec.ModuleActionContextRegistry; | ||
| import com.google.devtools.build.lib.exec.SpawnCache; | ||
|
|
@@ -90,5 +91,9 @@ public void registerSpawnStrategies( | |
| for (Map.Entry<RegexFilter, List<String>> entry : options.strategyByRegexp) { | ||
| registryBuilder.addDescriptionFilter(entry.getKey(), entry.getValue()); | ||
| } | ||
|
|
||
| for (Map.Entry<String, List<String>> strategy : options.allowedStrategiesByExecPlatform) { | ||
| registryBuilder.addExecPlatformFilter(Label.parseCanonicalUnchecked(strategy.getKey()), strategy.getValue()); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unchecked Label ParsingUsing parseCanonicalUnchecked without validation allows malformed label strings to cause runtime exceptions or unexpected behavior. Malicious configuration could exploit this to cause denial of service or system instability through invalid label injection. Standards
|
||
| } | ||
arvi18 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -22,6 +22,7 @@ | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import com.google.common.collect.ImmutableCollection; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import com.google.common.collect.ImmutableList; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import com.google.common.collect.ImmutableListMultimap; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import com.google.common.collect.ImmutableMap; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import com.google.common.collect.ImmutableMultimap; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import com.google.common.collect.LinkedListMultimap; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import com.google.common.collect.ListMultimap; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -34,6 +35,7 @@ | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import com.google.devtools.build.lib.actions.SandboxedSpawnStrategy; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import com.google.devtools.build.lib.actions.Spawn; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import com.google.devtools.build.lib.actions.SpawnStrategy; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import com.google.devtools.build.lib.cmdline.Label; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import com.google.devtools.build.lib.events.Event; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import com.google.devtools.build.lib.events.EventHandler; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import com.google.devtools.build.lib.events.Reporter; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -69,6 +71,7 @@ public final class SpawnStrategyRegistry | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| private final ImmutableListMultimap<String, SpawnStrategy> mnemonicToStrategies; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| private final StrategyRegexFilter strategyRegexFilter; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| private final StrategyPlatformFilter strategyPlatformFilter; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| private final ImmutableList<? extends SpawnStrategy> defaultStrategies; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| private final ImmutableMultimap<String, SandboxedSpawnStrategy> mnemonicToRemoteDynamicStrategies; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| private final ImmutableMultimap<String, SandboxedSpawnStrategy> mnemonicToLocalDynamicStrategies; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -77,12 +80,14 @@ public final class SpawnStrategyRegistry | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| private SpawnStrategyRegistry( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ImmutableListMultimap<String, SpawnStrategy> mnemonicToStrategies, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| StrategyRegexFilter strategyRegexFilter, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| StrategyPlatformFilter strategyPlatformFilter, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ImmutableList<? extends SpawnStrategy> defaultStrategies, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ImmutableMultimap<String, SandboxedSpawnStrategy> mnemonicToRemoteDynamicStrategies, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ImmutableMultimap<String, SandboxedSpawnStrategy> mnemonicToLocalDynamicStrategies, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| @Nullable AbstractSpawnStrategy remoteLocalFallbackStrategy) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| this.mnemonicToStrategies = mnemonicToStrategies; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| this.strategyRegexFilter = strategyRegexFilter; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| this.strategyPlatformFilter = strategyPlatformFilter; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| this.defaultStrategies = defaultStrategies; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| this.mnemonicToRemoteDynamicStrategies = mnemonicToRemoteDynamicStrategies; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| this.mnemonicToLocalDynamicStrategies = mnemonicToLocalDynamicStrategies; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -105,7 +110,8 @@ private SpawnStrategyRegistry( | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * using the given {@link Reporter}. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| public List<? extends SpawnStrategy> getStrategies(Spawn spawn, @Nullable EventHandler reporter) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return getStrategies(spawn.getResourceOwner(), spawn.getMnemonic(), reporter); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return strategyPlatformFilter.getStrategies( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| spawn, getStrategies(spawn.getResourceOwner(), spawn.getMnemonic(), reporter)); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
arvi18 marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -116,6 +122,8 @@ public List<? extends SpawnStrategy> getStrategies(Spawn spawn, @Nullable EventH | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * <p>If the reason for selecting the context is worth mentioning to the user, logs a message | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * using the given {@link Reporter}. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * NOTE: This method is public for Blaze, getStrategies(Spawn, EventHandler) must be used in Bazel. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| public List<? extends SpawnStrategy> getStrategies( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ActionExecutionMetadata resourceOwner, String mnemonic, @Nullable EventHandler reporter) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -154,7 +162,7 @@ public ImmutableCollection<SandboxedSpawnStrategy> getDynamicSpawnActionContexts | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ? mnemonicToRemoteDynamicStrategies | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| : mnemonicToLocalDynamicStrategies; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (mnemonicToDynamicStrategies.containsKey(spawn.getMnemonic())) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return mnemonicToDynamicStrategies.get(spawn.getMnemonic()); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return strategyPlatformFilter.getStrategies(spawn, mnemonicToDynamicStrategies.get(spawn.getMnemonic())); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (mnemonicToDynamicStrategies.containsKey("")) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return mnemonicToDynamicStrategies.get(""); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -164,8 +172,9 @@ public ImmutableCollection<SandboxedSpawnStrategy> getDynamicSpawnActionContexts | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| @Nullable | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| @Override | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| public AbstractSpawnStrategy getRemoteLocalFallbackStrategy() { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return remoteLocalFallbackStrategy; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| public AbstractSpawnStrategy getRemoteLocalFallbackStrategy(Spawn spawn) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return strategyPlatformFilter.getStrategies(spawn, Lists.newArrayList(remoteLocalFallbackStrategy)) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| .getFirst(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+175
to
+177
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. NoSuchElementException risk in remote fallback strategyIf remoteLocalFallbackStrategy is null, Lists.newArrayList will create an empty list, and calling getFirst() on an empty list will throw NoSuchElementException, causing runtime failures.
Suggested change
Standards
Comment on lines
+175
to
+177
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Potential NPE RiskThe method returns the first element from filtered strategies without checking if the list is empty. If all strategies are filtered out, getFirst() will throw a NoSuchElementException. This creates a reliability risk when no strategies match the platform filter criteria. Commitable Suggestion
Suggested change
Standards
Comment on lines
+175
to
+177
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Potential NullPointerExceptionThe method returns the first element from filtered strategies without checking if remoteLocalFallbackStrategy is null or if the filtered list is empty. This could lead to NullPointerException when remoteLocalFallbackStrategy is null or when all strategies are filtered out. Commitable Suggestion
Suggested change
Standards
Comment on lines
+176
to
+177
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Resource Leak RiskPotential null pointer exception when remoteLocalFallbackStrategy is null. The method creates a new ArrayList containing potentially null strategy without validation, then calls getFirst() which could fail if filtering returns empty list. This creates unreliable fallback behavior that could cause service degradation during remote execution failures. Standards
Comment on lines
+176
to
+177
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Strategy Collection LogicMethod calls getFirst() on filtered strategy list without null or empty validation. If strategyPlatformFilter returns empty list or remoteLocalFallbackStrategy is null, getFirst() will throw NoSuchElementException. Business logic requires fallback strategy availability guarantee. Standards
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -203,9 +212,16 @@ void logSpawnStrategies() { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| strategyRegexFilter.getFilterToStrategies().asMap().entrySet()) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Collection<SpawnStrategy> value = entry.getValue(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| logger.atInfo().log( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Inappropriate Logging LevelThe code uses INFO level for logging filter platform to strategy implementations. According to repository guidelines, a more appropriate logging level should be considered. This may lead to log pollution in production environments where this level of detail isn't necessary. Standards
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "FilterToStrategyImplementations: \"%s\" = [%s]", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "FilterDescriptionToStrategyImplementations: \"%s\" = [%s]", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| entry.getKey(), toImplementationNames(value)); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+215
to
216
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Inappropriate Logging LevelThe code uses logger.atInfo() for logging filter strategy implementations. According to repository guidelines, a more appropriate logging level should be considered for this type of diagnostic information, likely debug level would be more suitable for detailed implementation mappings. Standards
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| for (Map.Entry<Label, ImmutableList<SpawnStrategy>> entry : | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| strategyPlatformFilter.getFilterToStrategies().entrySet()) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Collection<SpawnStrategy> value = entry.getValue(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| logger.atInfo().log( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "FilterPlatformToStrategyImplementations: \"%s\" = [%s]", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| entry.getKey().getCanonicalName(), toImplementationNames(value)); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
arvi18 marked this conversation as resolved.
Show resolved
Hide resolved
Comment on lines
+221
to
+223
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Inappropriate Logging LevelRepository guideline violation: using INFO level for platform filtering logging may not be appropriate. This implementation logs platform-to-strategy mappings at INFO level which could create excessive log noise in production environments. Consider using DEBUG or TRACE level for detailed implementation mappings to improve system observability without overwhelming logs. Standards
Comment on lines
+221
to
+223
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Inappropriate Logging LevelRepository guideline violation: using INFO level for platform filter logging may not be appropriate. Platform filtering configuration details might be better suited for DEBUG level to reduce noise in production logs while maintaining debugging capability. Standards
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| logger.atInfo().log( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "DefaultStrategyImplementations: [%s]", toImplementationNames(defaultStrategies)); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -287,6 +303,7 @@ public static final class Builder { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| private final HashMap<String, List<String>> mnemonicToRemoteDynamicIdentifiers = | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| new HashMap<>(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| private final HashMap<String, List<String>> mnemonicToLocalDynamicIdentifiers = new HashMap<>(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| private final HashMap<Label, List<String>> execPlatformFilters = new HashMap<>(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| @Nullable private String remoteLocalFallbackStrategyIdentifier; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -314,6 +331,12 @@ public Builder addDescriptionFilter(RegexFilter filter, List<String> identifiers | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return this; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| @CanIgnoreReturnValue | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| public Builder addExecPlatformFilter(Label execPlatform, List<String> identifiers) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| this.execPlatformFilters.put(execPlatform, identifiers); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return this; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * Adds a filter limiting any spawn whose {@linkplain Spawn#getMnemonic() mnemonic} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * (case-sensitively) matches the given mnemonic to only use strategies with the given | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -444,6 +467,14 @@ public SpawnStrategyRegistry build() throws AbruptExitException { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ImmutableMap.Builder<Label, ImmutableList<SpawnStrategy>> platformToStrategies = ImmutableMap.builder(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| for (Map.Entry<Label, List<String>> entry : execPlatformFilters.entrySet()) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Label platform = entry.getKey(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| platformToStrategies.put( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| platform, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| strategyMapper.toStrategies(entry.getValue(), "platform " + platform.getCanonicalName())); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ImmutableListMultimap.Builder<String, SpawnStrategy> mnemonicToStrategies = | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| new ImmutableListMultimap.Builder<>(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| for (Map.Entry<String, List<String>> entry : mnemonicToIdentifiers.entrySet()) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -514,6 +545,7 @@ public SpawnStrategyRegistry build() throws AbruptExitException { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| mnemonicToStrategies.build(), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| new StrategyRegexFilter( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| strategyMapper, strategyPolicy, filterToIdentifiers, filterToStrategies), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| new StrategyPlatformFilter(strategyMapper, platformToStrategies.build()), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| defaultStrategies, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| mnemonicToRemoteStrategies.build(), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| mnemonicToLocalStrategies.build(), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -595,6 +627,46 @@ public String toString() { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| private static class StrategyPlatformFilter { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| private final StrategyMapper strategyMapper; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| private final ImmutableMap<Label, ImmutableList<SpawnStrategy>> platformToStrategies; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| private StrategyPlatformFilter( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| StrategyMapper strategyMapper, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ImmutableMap<Label, ImmutableList<SpawnStrategy>> platformToStrategies) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| this.strategyMapper = strategyMapper; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| this.platformToStrategies = platformToStrategies; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| public <T extends SpawnStrategy> List<T> getStrategies( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Spawn spawn, List<T> candidateStrategies) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| var platformLabel = spawn.getExecutionPlatformLabel(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
arvi18 marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Preconditions.checkNotNull(platformLabel, "Attempting to spawn action without an execution platform."); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
arvi18 marked this conversation as resolved.
Show resolved
Hide resolved
arvi18 marked this conversation as resolved.
Show resolved
Hide resolved
arvi18 marked this conversation as resolved.
Show resolved
Hide resolved
arvi18 marked this conversation as resolved.
Show resolved
Hide resolved
arvi18 marked this conversation as resolved.
Show resolved
Hide resolved
arvi18 marked this conversation as resolved.
Show resolved
Hide resolved
arvi18 marked this conversation as resolved.
Show resolved
Hide resolved
arvi18 marked this conversation as resolved.
Show resolved
Hide resolved
arvi18 marked this conversation as resolved.
Show resolved
Hide resolved
arvi18 marked this conversation as resolved.
Show resolved
Hide resolved
arvi18 marked this conversation as resolved.
Show resolved
Hide resolved
arvi18 marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Null pointer risk in platform label validationThe null check for platformLabel will throw NPE when a spawn has no execution platform. This contradicts the existing code in getPlatformProto that handles null execution platforms gracefully.
Suggested change
Standards
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missing Null CheckCode directly calls checkNotNull without first checking if platformLabel is null. This violates the repository guideline to check for null before using Preconditions.checkNotNull. This could lead to unnecessary exceptions when a simple null check could handle the case more gracefully. Commitable Suggestion
Suggested change
Standards
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missing Null CheckNull check is performed using Preconditions.checkNotNull without first checking if platformLabel is null. This violates the repository guideline requiring a null check before using Preconditions.checkNotNull. This could lead to unnecessary exceptions when a simple null check could handle the case more gracefully. Commitable Suggestion
Suggested change
Standards
Comment on lines
+643
to
+644
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missing Null CheckRepository guideline violation: null check condition should be applied before Preconditions.checkNotNull. The current implementation retrieves platformLabel and immediately passes it to checkNotNull without validation, which could cause unclear error messages if the value is null. Commitable Suggestion
Suggested change
Standards
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| var allowedStrategies = platformToStrategies.get(platformLabel); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
arvi18 marked this conversation as resolved.
Show resolved
Hide resolved
arvi18 marked this conversation as resolved.
Show resolved
Hide resolved
arvi18 marked this conversation as resolved.
Show resolved
Hide resolved
arvi18 marked this conversation as resolved.
Show resolved
Hide resolved
arvi18 marked this conversation as resolved.
Show resolved
Hide resolved
arvi18 marked this conversation as resolved.
Show resolved
Hide resolved
arvi18 marked this conversation as resolved.
Show resolved
Hide resolved
arvi18 marked this conversation as resolved.
Show resolved
Hide resolved
arvi18 marked this conversation as resolved.
Show resolved
Hide resolved
arvi18 marked this conversation as resolved.
Show resolved
Hide resolved
Comment on lines
+638
to
+646
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Potential N+1 Performance Issue in Platform Filter Strategy SelectionThe current implementation iterates through all candidate strategies for each spawn, checking if each strategy is in the allowed strategies list. This creates an O(n*m) operation where n is the number of candidate strategies and m is the number of allowed strategies. For platforms with many strategies and frequent spawns, this could create unnecessary CPU overhead. Additionally, the linear search pattern can be inefficient for large strategy collections. var allowedStrategies = platformToStrategies.get(platformLabel);
if (allowedStrategies != null) {
// Convert to HashSet for O(1) lookups instead of O(n) contains() operations
Set<SpawnStrategy> allowedStrategiesSet = new HashSet<>(allowedStrategies);
List<T> filteredStrategies = new ArrayList<>();
for (var strategy : candidateStrategies) {
if (allowedStrategiesSet.contains(strategy)) {
filteredStrategies.add(strategy);
}
}
// If filtering resulted in an empty list, return original candidates
// to ensure at least one strategy is available
return filteredStrategies.isEmpty() ? candidateStrategies : filteredStrategies;
}ReferencesStandard: Google's Core Performance Principles - Hot Path Optimization |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (allowedStrategies != null) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| List<T> filteredStrategies = new ArrayList<>(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| for (var strategy : candidateStrategies) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (allowedStrategies.contains(strategy)) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| filteredStrategies.add(strategy); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
arvi18 marked this conversation as resolved.
Show resolved
Hide resolved
arvi18 marked this conversation as resolved.
Show resolved
Hide resolved
arvi18 marked this conversation as resolved.
Show resolved
Hide resolved
arvi18 marked this conversation as resolved.
Show resolved
Hide resolved
arvi18 marked this conversation as resolved.
Show resolved
Hide resolved
arvi18 marked this conversation as resolved.
Show resolved
Hide resolved
arvi18 marked this conversation as resolved.
Show resolved
Hide resolved
arvi18 marked this conversation as resolved.
Show resolved
Hide resolved
arvi18 marked this conversation as resolved.
Show resolved
Hide resolved
arvi18 marked this conversation as resolved.
Show resolved
Hide resolved
arvi18 marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return filteredStrategies; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return candidateStrategies; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| public <T extends SpawnStrategy> ImmutableCollection<T> getStrategies( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Spawn spawn, ImmutableCollection<T> candidateStrategies) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return ImmutableList.copyOf(getStrategies(spawn, Lists.newCopyOnWriteArrayList(candidateStrategies))); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ImmutableMap<Label, ImmutableList<SpawnStrategy>> getFilterToStrategies() { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return platformToStrategies; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /* Maps the strategy identifier (e.g. "local", "worker"..) to the real strategy. */ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| private static class StrategyMapper { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Uh oh!
There was an error while loading. Please reload this page.