diff --git a/src/main/java/com/google/devtools/build/lib/worker/WorkerOptions.java b/src/main/java/com/google/devtools/build/lib/worker/WorkerOptions.java index cb3c4568e0ecbc..b03c3d0817f682 100644 --- a/src/main/java/com/google/devtools/build/lib/worker/WorkerOptions.java +++ b/src/main/java/com/google/devtools/build/lib/worker/WorkerOptions.java @@ -49,21 +49,19 @@ public class WorkerOptions extends OptionsBase { /** * Defines a resource converter for named values in the form [name=]value, where the value is * {@link ResourceConverter.FLAG_SYNTAX}. If no name is provided (used when setting a default), - * the empty string is used as the key. The default value for unspecified mnemonics is {@value - * DEFAULT_VALUE}. "auto" currently returns the default. + * the empty string is used as the key. The default value for unspecified mnemonics is defined in + * {@link WorkerPool.createWorkerPools}. "auto" currently returns the default. */ public static class MultiResourceConverter extends Converter.Contextless> { - public static final int DEFAULT_VALUE = 4; - - static ResourceConverter valueConverter = - new ResourceConverter(() -> DEFAULT_VALUE, 0, Integer.MAX_VALUE); + static final ResourceConverter valueConverter = + new ResourceConverter(() -> 0, 0, Integer.MAX_VALUE); @Override public Map.Entry convert(String input) throws OptionsParsingException { // TODO(steinman): Make auto value return a reasonable multiplier of host capacity. - if (input == null || "null".equals(input)) { - input = "auto"; + if (input == null || "null".equals(input) || "auto".equals(input)) { + return Maps.immutableEntry(null, null); } int pos = input.indexOf('='); if (pos < 0) { @@ -71,6 +69,10 @@ public Map.Entry convert(String input) throws OptionsParsingExc } String name = input.substring(0, pos); String value = input.substring(pos + 1); + if ("auto".equals(value)) { + return Maps.immutableEntry(name, null); + } + return Maps.immutableEntry(name, valueConverter.convert(value, /*conversionContext=*/ null)); } diff --git a/src/main/java/com/google/devtools/build/lib/worker/WorkerPool.java b/src/main/java/com/google/devtools/build/lib/worker/WorkerPool.java index 180c52883950e2..e19f2b21fbef1a 100644 --- a/src/main/java/com/google/devtools/build/lib/worker/WorkerPool.java +++ b/src/main/java/com/google/devtools/build/lib/worker/WorkerPool.java @@ -16,7 +16,6 @@ import com.google.common.base.Throwables; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; -import com.google.devtools.build.lib.worker.WorkerOptions.MultiResourceConverter; import java.io.IOException; import java.util.LinkedHashMap; import java.util.List; @@ -64,15 +63,14 @@ public WorkerPool(WorkerPoolConfig workerPoolConfig) { highPriorityWorkerMnemonics = ImmutableSet.copyOf((Iterable) workerPoolConfig.getHighPriorityWorkers()); - Map config = createConfigFromOptions(workerPoolConfig.getWorkerMaxInstances()); - Map multiplexConfig = - createConfigFromOptions(workerPoolConfig.getWorkerMaxMultiplexInstances()); + ImmutableMap config = + createConfigFromOptions(workerPoolConfig.getWorkerMaxInstances(), DEFAULT_MAX_WORKERS); + ImmutableMap multiplexConfig = + createConfigFromOptions( + workerPoolConfig.getWorkerMaxMultiplexInstances(), DEFAULT_MAX_MULTIPLEX_WORKERS); - workerPools = - createWorkerPools(workerPoolConfig.getWorkerFactory(), config, DEFAULT_MAX_WORKERS); - multiplexPools = - createWorkerPools( - workerPoolConfig.getWorkerFactory(), multiplexConfig, DEFAULT_MAX_MULTIPLEX_WORKERS); + workerPools = createWorkerPools(workerPoolConfig.getWorkerFactory(), config); + multiplexPools = createWorkerPools(workerPoolConfig.getWorkerFactory(), multiplexConfig); } public WorkerPoolConfig getWorkerPoolConfig() { @@ -85,29 +83,28 @@ public WorkerPoolConfig getWorkerPoolConfig() { */ @Nonnull private static ImmutableMap createConfigFromOptions( - List> options) { + List> options, int defaultMaxWorkers) { LinkedHashMap newConfigBuilder = new LinkedHashMap<>(); for (Map.Entry entry : options) { - newConfigBuilder.put(entry.getKey(), entry.getValue()); + if (entry.getValue() != null) { + newConfigBuilder.put(entry.getKey(), entry.getValue()); + } else if (entry.getKey() != null) { + newConfigBuilder.put(entry.getKey(), defaultMaxWorkers); + } } - if (!newConfigBuilder.containsKey("")) { // Empty string gives the number of workers for any type of worker not explicitly specified. - // If no value is given, use the default, 2. - newConfigBuilder.put("", MultiResourceConverter.DEFAULT_VALUE); + // If no value is given, use the default. + newConfigBuilder.put("", defaultMaxWorkers); } - return ImmutableMap.copyOf(newConfigBuilder); } private static ImmutableMap createWorkerPools( - WorkerFactory factory, Map config, int defaultMaxWorkers) { + WorkerFactory factory, Map config) { ImmutableMap.Builder workerPoolsBuilder = ImmutableMap.builder(); config.forEach( (key, value) -> workerPoolsBuilder.put(key, new SimpleWorkerPool(factory, value))); - if (!config.containsKey("")) { - workerPoolsBuilder.put("", new SimpleWorkerPool(factory, defaultMaxWorkers)); - } return workerPoolsBuilder.build(); } diff --git a/src/test/java/com/google/devtools/build/lib/worker/MultiResourceConverterTest.java b/src/test/java/com/google/devtools/build/lib/worker/MultiResourceConverterTest.java index 5956ec92f03762..d5deb2bafedc47 100644 --- a/src/test/java/com/google/devtools/build/lib/worker/MultiResourceConverterTest.java +++ b/src/test/java/com/google/devtools/build/lib/worker/MultiResourceConverterTest.java @@ -38,8 +38,7 @@ public void setUp() { @Test public void convert_mnemonicEqualsAuto_returnsDefault() throws OptionsParsingException { - assertThat(multiResourceConverter.convert("someMnemonic=auto").getValue()) - .isEqualTo(MultiResourceConverter.DEFAULT_VALUE); + assertThat(multiResourceConverter.convert("someMnemonic=auto").getValue()).isNull(); } @Test @@ -51,8 +50,7 @@ public void convert_mnemonicEqualsKeyword_equalsResourceConverterConvertKeyword( @Test public void convert_auto_returnsDefault() throws OptionsParsingException { - assertThat(multiResourceConverter.convert("auto").getValue()) - .isEqualTo(MultiResourceConverter.DEFAULT_VALUE); + assertThat(multiResourceConverter.convert("auto").getValue()).isNull(); } @Test @@ -70,6 +68,6 @@ public void convert_mnemonic_savesCorrectKey() throws Exception { @Test public void convert_auto_setsEmptyStringAKADefaultAsKey() throws Exception { - assertThat(multiResourceConverter.convert("auto").getKey()).isEmpty(); + assertThat(multiResourceConverter.convert("auto").getKey()).isNull(); } }