Repository: aurora Updated Branches: refs/heads/master 8e5e08ebf -> 5b34231ba
Custom converter to allow the -thermos_executor_resources flag to take an empty string and parse it to an empty list Fixes the issue that caused the voting to fail for the 0.19.0 Aurora packages. Fix cribbed from: https://github.com/cbeust/jcommander/pull/422 Implemented as a custom converter as suggested here: https://reviews.apache.org/r/64824/ Reviewed at https://reviews.apache.org/r/64934/ Project: http://git-wip-us.apache.org/repos/asf/aurora/repo Commit: http://git-wip-us.apache.org/repos/asf/aurora/commit/5b34231b Tree: http://git-wip-us.apache.org/repos/asf/aurora/tree/5b34231b Diff: http://git-wip-us.apache.org/repos/asf/aurora/diff/5b34231b Branch: refs/heads/master Commit: 5b34231ba90f01a08c10bd9b819437d892910c5c Parents: 8e5e08e Author: Renan DelValle <[email protected]> Authored: Tue Jan 9 13:27:28 2018 -0800 Committer: Bill Farner <[email protected]> Committed: Tue Jan 9 13:27:28 2018 -0800 ---------------------------------------------------------------------- .../apache/aurora/scheduler/app/AppModule.java | 7 +- .../config/splitters/CommaSplitter.java | 34 ++++++++++ .../configuration/executor/ExecutorModule.java | 7 +- .../discovery/FlaggedZooKeeperConfig.java | 4 +- .../http/api/security/HttpSecurityModule.java | 4 +- .../scheduler/offers/OfferManagerModule.java | 7 +- .../scheduler/preemptor/PreemptorModule.java | 4 +- .../scheduling/TaskAssignerImplModule.java | 4 +- .../apache/aurora/scheduler/sla/SlaModule.java | 7 +- .../aurora/scheduler/state/StateModule.java | 4 +- .../aurora/scheduler/thrift/aop/AopModule.java | 5 +- .../scheduler/config/CommandLineTest.java | 71 ++++++++++++++++++++ 12 files changed, 144 insertions(+), 14 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/aurora/blob/5b34231b/src/main/java/org/apache/aurora/scheduler/app/AppModule.java ---------------------------------------------------------------------- diff --git a/src/main/java/org/apache/aurora/scheduler/app/AppModule.java b/src/main/java/org/apache/aurora/scheduler/app/AppModule.java index 817a019..ffc0744 100644 --- a/src/main/java/org/apache/aurora/scheduler/app/AppModule.java +++ b/src/main/java/org/apache/aurora/scheduler/app/AppModule.java @@ -37,6 +37,7 @@ import org.apache.aurora.scheduler.SchedulerServicesModule; import org.apache.aurora.scheduler.app.SchedulerMain.Options.DriverKind; import org.apache.aurora.scheduler.async.AsyncModule; import org.apache.aurora.scheduler.config.CliOptions; +import org.apache.aurora.scheduler.config.splitters.CommaSplitter; import org.apache.aurora.scheduler.config.validators.PositiveNumber; import org.apache.aurora.scheduler.configuration.ConfigurationManager; import org.apache.aurora.scheduler.configuration.ConfigurationManager.ConfigurationManagerSettings; @@ -85,7 +86,8 @@ public class AppModule extends AbstractModule { // parameters that define an arity. You will have to convert these values yourself..." @Parameter(names = "-allowed_container_types", - description = "Container types that are allowed to be used by jobs.") + description = "Container types that are allowed to be used by jobs.", + splitter = CommaSplitter.class) public List<_Fields> allowedContainerTypes = ImmutableList.of(Container._Fields.MESOS); @Parameter(names = "-allow_docker_parameters", @@ -95,7 +97,8 @@ public class AppModule extends AbstractModule { @Parameter(names = "-default_docker_parameters", description = - "Default docker parameters for any job that does not explicitly declare parameters.") + "Default docker parameters for any job that does not explicitly declare parameters.", + splitter = CommaSplitter.class) public List<DockerParameter> defaultDockerParameters = ImmutableList.of(); @Parameter(names = "-require_docker_use_executor", http://git-wip-us.apache.org/repos/asf/aurora/blob/5b34231b/src/main/java/org/apache/aurora/scheduler/config/splitters/CommaSplitter.java ---------------------------------------------------------------------- diff --git a/src/main/java/org/apache/aurora/scheduler/config/splitters/CommaSplitter.java b/src/main/java/org/apache/aurora/scheduler/config/splitters/CommaSplitter.java new file mode 100644 index 0000000..d1ec962 --- /dev/null +++ b/src/main/java/org/apache/aurora/scheduler/config/splitters/CommaSplitter.java @@ -0,0 +1,34 @@ +/** + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.aurora.scheduler.config.splitters; + +import java.util.Arrays; +import java.util.Collections; +import java.util.List; + +import com.beust.jcommander.converters.IParameterSplitter; + +// Workaround for default comma splitter returning [""] when an empty +// string is passed as an argument. This custom splitter may be removed +// after https://github.com/cbeust/jcommander/pull/422 has landed. +public class CommaSplitter implements IParameterSplitter { + @Override + public List<String> split(String value) { + if ("".equals(value) || "''".equals(value)) { + return Collections.emptyList(); + } + return Arrays.asList(value.split(",")); + } +} http://git-wip-us.apache.org/repos/asf/aurora/blob/5b34231b/src/main/java/org/apache/aurora/scheduler/configuration/executor/ExecutorModule.java ---------------------------------------------------------------------- diff --git a/src/main/java/org/apache/aurora/scheduler/configuration/executor/ExecutorModule.java b/src/main/java/org/apache/aurora/scheduler/configuration/executor/ExecutorModule.java index 76ce39e..e610112 100644 --- a/src/main/java/org/apache/aurora/scheduler/configuration/executor/ExecutorModule.java +++ b/src/main/java/org/apache/aurora/scheduler/configuration/executor/ExecutorModule.java @@ -34,6 +34,7 @@ import org.apache.aurora.common.base.MorePreconditions; import org.apache.aurora.common.quantity.Data; import org.apache.aurora.gen.Volume; import org.apache.aurora.gen.apiConstants; +import org.apache.aurora.scheduler.config.splitters.CommaSplitter; import org.apache.aurora.scheduler.config.types.DataAmount; import org.apache.aurora.scheduler.config.validators.ReadableFile; import org.apache.aurora.scheduler.resources.ResourceType; @@ -68,7 +69,8 @@ public class ExecutorModule extends AbstractModule { @Parameter(names = "-thermos_executor_resources", description = "A comma separated list of additional resources to copy into the sandbox." + "Note: if thermos_executor_path is not the thermos_executor.pex file itself, " - + "this must include it.") + + "this must include it.", + splitter = CommaSplitter.class) public List<String> thermosExecutorResources = ImmutableList.of(); @Parameter(names = "-thermos_executor_flags", @@ -98,7 +100,8 @@ public class ExecutorModule extends AbstractModule { @Parameter(names = "-global_container_mounts", description = "A comma separated list of mount points (in host:container form) to mount " - + "into all (non-mesos) containers.") + + "into all (non-mesos) containers.", + splitter = CommaSplitter.class) public List<Volume> globalContainerMounts = ImmutableList.of(); @Parameter(names = "-populate_discovery_info", http://git-wip-us.apache.org/repos/asf/aurora/blob/5b34231b/src/main/java/org/apache/aurora/scheduler/discovery/FlaggedZooKeeperConfig.java ---------------------------------------------------------------------- diff --git a/src/main/java/org/apache/aurora/scheduler/discovery/FlaggedZooKeeperConfig.java b/src/main/java/org/apache/aurora/scheduler/discovery/FlaggedZooKeeperConfig.java index 694a4fc..abab213 100644 --- a/src/main/java/org/apache/aurora/scheduler/discovery/FlaggedZooKeeperConfig.java +++ b/src/main/java/org/apache/aurora/scheduler/discovery/FlaggedZooKeeperConfig.java @@ -26,6 +26,7 @@ import com.google.common.collect.ImmutableList; import org.apache.aurora.common.quantity.Amount; import org.apache.aurora.common.zookeeper.Credentials; +import org.apache.aurora.scheduler.config.splitters.CommaSplitter; import org.apache.aurora.scheduler.config.types.TimeAmount; import org.apache.aurora.scheduler.config.validators.NotEmptyIterable; @@ -51,7 +52,8 @@ public final class FlaggedZooKeeperConfig { names = "-zk_endpoints", required = true, validateValueWith = NotEmptyIterable.class, - description = "Endpoint specification for the ZooKeeper servers.") + description = "Endpoint specification for the ZooKeeper servers.", + splitter = CommaSplitter.class) public List<InetSocketAddress> zkEndpoints; @Parameter(names = "-zk_chroot_path", http://git-wip-us.apache.org/repos/asf/aurora/blob/5b34231b/src/main/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityModule.java ---------------------------------------------------------------------- diff --git a/src/main/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityModule.java b/src/main/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityModule.java index 9d757db..ddcc093 100644 --- a/src/main/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityModule.java +++ b/src/main/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityModule.java @@ -44,6 +44,7 @@ import org.apache.aurora.gen.AuroraAdmin; import org.apache.aurora.gen.AuroraSchedulerManager; import org.apache.aurora.scheduler.app.MoreModules; import org.apache.aurora.scheduler.config.CliOptions; +import org.apache.aurora.scheduler.config.splitters.CommaSplitter; import org.apache.aurora.scheduler.http.api.security.HttpSecurityModule.Options.HttpAuthenticationMechanism; import org.apache.aurora.scheduler.thrift.aop.AnnotatedAuroraAdmin; import org.apache.shiro.SecurityUtils; @@ -76,7 +77,8 @@ public class HttpSecurityModule extends ServletModule { @Parameters(separators = "=") public static class Options { @Parameter(names = "-shiro_realm_modules", - description = "Guice modules for configuring Shiro Realms.") + description = "Guice modules for configuring Shiro Realms.", + splitter = CommaSplitter.class) @SuppressWarnings("rawtypes") public List<Class> shiroRealmModule = ImmutableList.of(IniShiroRealmModule.class); http://git-wip-us.apache.org/repos/asf/aurora/blob/5b34231b/src/main/java/org/apache/aurora/scheduler/offers/OfferManagerModule.java ---------------------------------------------------------------------- diff --git a/src/main/java/org/apache/aurora/scheduler/offers/OfferManagerModule.java b/src/main/java/org/apache/aurora/scheduler/offers/OfferManagerModule.java index 823c369..de16c14 100644 --- a/src/main/java/org/apache/aurora/scheduler/offers/OfferManagerModule.java +++ b/src/main/java/org/apache/aurora/scheduler/offers/OfferManagerModule.java @@ -37,6 +37,7 @@ import org.apache.aurora.common.quantity.Time; import org.apache.aurora.common.util.Random; import org.apache.aurora.scheduler.app.MoreModules; import org.apache.aurora.scheduler.config.CliOptions; +import org.apache.aurora.scheduler.config.splitters.CommaSplitter; import org.apache.aurora.scheduler.config.types.TimeAmount; import org.apache.aurora.scheduler.config.validators.NotNegativeAmount; import org.apache.aurora.scheduler.config.validators.NotNegativeNumber; @@ -91,11 +92,13 @@ public class OfferManagerModule extends AbstractModule { description = "Iteration order for offers, to influence task scheduling. Multiple orderings will be " + "compounded together. E.g. CPU,MEMORY,RANDOM would sort first by cpus offered," - + " then memory and finally would randomize any equal offers.") + + " then memory and finally would randomize any equal offers.", + splitter = CommaSplitter.class) public List<OfferOrder> offerOrder = ImmutableList.of(OfferOrder.RANDOM); @Parameter(names = "-offer_order_modules", - description = "Custom Guice module to provide an offer ordering.") + description = "Custom Guice module to provide an offer ordering.", + splitter = CommaSplitter.class) @SuppressWarnings("rawtypes") public List<Class> offerOrderModules = ImmutableList.of(OfferOrderModule.class); http://git-wip-us.apache.org/repos/asf/aurora/blob/5b34231b/src/main/java/org/apache/aurora/scheduler/preemptor/PreemptorModule.java ---------------------------------------------------------------------- diff --git a/src/main/java/org/apache/aurora/scheduler/preemptor/PreemptorModule.java b/src/main/java/org/apache/aurora/scheduler/preemptor/PreemptorModule.java index 007ebc2..4de5ef8 100644 --- a/src/main/java/org/apache/aurora/scheduler/preemptor/PreemptorModule.java +++ b/src/main/java/org/apache/aurora/scheduler/preemptor/PreemptorModule.java @@ -37,6 +37,7 @@ import org.apache.aurora.scheduler.SchedulerServicesModule; import org.apache.aurora.scheduler.app.MoreModules; import org.apache.aurora.scheduler.base.TaskGroupKey; import org.apache.aurora.scheduler.config.CliOptions; +import org.apache.aurora.scheduler.config.splitters.CommaSplitter; import org.apache.aurora.scheduler.config.types.TimeAmount; import org.apache.aurora.scheduler.config.validators.PositiveNumber; import org.apache.aurora.scheduler.events.PubsubEventModule; @@ -80,7 +81,8 @@ public class PreemptorModule extends AbstractModule { public int reservationMaxBatchSize = 5; @Parameter(names = "-preemption_slot_finder_modules", - description = "Guice modules for custom preemption slot searching for pending tasks.") + description = "Guice modules for custom preemption slot searching for pending tasks.", + splitter = CommaSplitter.class) @SuppressWarnings("rawtypes") public List<Class> slotFinderModules = ImmutableList.of( PendingTaskProcessorModule.class, http://git-wip-us.apache.org/repos/asf/aurora/blob/5b34231b/src/main/java/org/apache/aurora/scheduler/scheduling/TaskAssignerImplModule.java ---------------------------------------------------------------------- diff --git a/src/main/java/org/apache/aurora/scheduler/scheduling/TaskAssignerImplModule.java b/src/main/java/org/apache/aurora/scheduler/scheduling/TaskAssignerImplModule.java index 2ddd4f5..d101a49 100644 --- a/src/main/java/org/apache/aurora/scheduler/scheduling/TaskAssignerImplModule.java +++ b/src/main/java/org/apache/aurora/scheduler/scheduling/TaskAssignerImplModule.java @@ -24,6 +24,7 @@ import com.google.inject.Module; import org.apache.aurora.scheduler.app.MoreModules; import org.apache.aurora.scheduler.config.CliOptions; +import org.apache.aurora.scheduler.config.splitters.CommaSplitter; /** * The default TaskAssigner implementation that allows the injection of custom offer @@ -34,7 +35,8 @@ public class TaskAssignerImplModule extends AbstractModule { @Parameters(separators = "=") public static class Options { @Parameter(names = "-offer_selector_modules", - description = "Guice module for customizing the TaskAssignerImpl's OfferSelector.") + description = "Guice module for customizing the TaskAssignerImpl's OfferSelector.", + splitter = CommaSplitter.class) @SuppressWarnings("rawtypes") public List<Class> offerSelectorModules = ImmutableList.of(FirstFitOfferSelectorModule.class); http://git-wip-us.apache.org/repos/asf/aurora/blob/5b34231b/src/main/java/org/apache/aurora/scheduler/sla/SlaModule.java ---------------------------------------------------------------------- diff --git a/src/main/java/org/apache/aurora/scheduler/sla/SlaModule.java b/src/main/java/org/apache/aurora/scheduler/sla/SlaModule.java index 7c8d2df..25ed474 100644 --- a/src/main/java/org/apache/aurora/scheduler/sla/SlaModule.java +++ b/src/main/java/org/apache/aurora/scheduler/sla/SlaModule.java @@ -34,6 +34,7 @@ import com.google.inject.Singleton; import org.apache.aurora.common.quantity.Time; import org.apache.aurora.scheduler.SchedulerServicesModule; import org.apache.aurora.scheduler.base.AsyncUtil; +import org.apache.aurora.scheduler.config.splitters.CommaSplitter; import org.apache.aurora.scheduler.config.types.TimeAmount; import org.apache.aurora.scheduler.config.validators.PositiveAmount; import org.apache.aurora.scheduler.sla.MetricCalculator.MetricCalculatorSettings; @@ -66,12 +67,14 @@ public class SlaModule extends AbstractModule { public TimeAmount slaRefreshInterval = new TimeAmount(1, Time.MINUTES); @Parameter(names = "-sla_prod_metrics", - description = "Metric categories collected for production tasks.") + description = "Metric categories collected for production tasks.", + splitter = CommaSplitter.class) public List<MetricCategory> slaProdMetrics = ImmutableList.of(JOB_UPTIMES, PLATFORM_UPTIME, MEDIANS); @Parameter(names = "-sla_non_prod_metrics", - description = "Metric categories collected for non production tasks.") + description = "Metric categories collected for non production tasks.", + splitter = CommaSplitter.class) public List<MetricCategory> slaNonProdMetrics = ImmutableList.of(); } http://git-wip-us.apache.org/repos/asf/aurora/blob/5b34231b/src/main/java/org/apache/aurora/scheduler/state/StateModule.java ---------------------------------------------------------------------- diff --git a/src/main/java/org/apache/aurora/scheduler/state/StateModule.java b/src/main/java/org/apache/aurora/scheduler/state/StateModule.java index 46e9227..76c3277 100644 --- a/src/main/java/org/apache/aurora/scheduler/state/StateModule.java +++ b/src/main/java/org/apache/aurora/scheduler/state/StateModule.java @@ -26,6 +26,7 @@ import com.google.inject.Module; import org.apache.aurora.scheduler.app.MoreModules; import org.apache.aurora.scheduler.config.CliOptions; +import org.apache.aurora.scheduler.config.splitters.CommaSplitter; import org.apache.aurora.scheduler.events.PubsubEventModule; import org.apache.aurora.scheduler.mesos.MesosTaskFactory; import org.apache.aurora.scheduler.mesos.MesosTaskFactory.MesosTaskFactoryImpl; @@ -41,7 +42,8 @@ public class StateModule extends AbstractModule { @Parameters(separators = "=") public static class Options { @Parameter(names = "-task_assigner_modules", - description = "Guice modules for customizing task assignment.") + description = "Guice modules for customizing task assignment.", + splitter = CommaSplitter.class) @SuppressWarnings("rawtypes") public List<Class> taskAssignerModules = ImmutableList.of(TaskAssignerImplModule.class); } http://git-wip-us.apache.org/repos/asf/aurora/blob/5b34231b/src/main/java/org/apache/aurora/scheduler/thrift/aop/AopModule.java ---------------------------------------------------------------------- diff --git a/src/main/java/org/apache/aurora/scheduler/thrift/aop/AopModule.java b/src/main/java/org/apache/aurora/scheduler/thrift/aop/AopModule.java index a66d804..476129a 100644 --- a/src/main/java/org/apache/aurora/scheduler/thrift/aop/AopModule.java +++ b/src/main/java/org/apache/aurora/scheduler/thrift/aop/AopModule.java @@ -28,6 +28,7 @@ import org.aopalliance.intercept.MethodInterceptor; import org.apache.aurora.gen.Response; import org.apache.aurora.scheduler.app.MoreModules; import org.apache.aurora.scheduler.config.CliOptions; +import org.apache.aurora.scheduler.config.splitters.CommaSplitter; import org.apache.aurora.scheduler.thrift.auth.DecoratedThrift; /** @@ -38,7 +39,9 @@ public class AopModule extends AbstractModule { @Parameters(separators = "=") public static class Options { @Parameter(names = "-thrift_method_interceptor_modules", - description = "Custom Guice module(s) to provide additional Thrift method interceptors.") + description = "Custom Guice module(s) to provide additional Thrift method interceptors.", + splitter = CommaSplitter.class) + @SuppressWarnings("rawtypes") public List<Class> methodInterceptorModules = ImmutableList.of(); } http://git-wip-us.apache.org/repos/asf/aurora/blob/5b34231b/src/test/java/org/apache/aurora/scheduler/config/CommandLineTest.java ---------------------------------------------------------------------- diff --git a/src/test/java/org/apache/aurora/scheduler/config/CommandLineTest.java b/src/test/java/org/apache/aurora/scheduler/config/CommandLineTest.java index f685d2e..c84fad3 100644 --- a/src/test/java/org/apache/aurora/scheduler/config/CommandLineTest.java +++ b/src/test/java/org/apache/aurora/scheduler/config/CommandLineTest.java @@ -390,6 +390,77 @@ public class CommandLineTest { assertEqualOptions(expected, parsed); } + @Test + public void testEmptyListOptions() { + + // This test ensures that all list options are capable of returning + // empty lists. + + CliOptions expected = new CliOptions(); + expected.main.clusterName = "testing"; + expected.driver.mesosMasterAddress = "testing"; + expected.backup.backupDir = new File("testing"); + expected.main.serversetPath = "testing"; + expected.zk.zkEndpoints = ImmutableList.of(InetSocketAddress.createUnresolved("testing", 42)); + expected.offer.offerOrder = ImmutableList.of(); + expected.offer.offerOrderModules = ImmutableList.of(); + expected.executor.thermosExecutorResources = ImmutableList.of(); + expected.executor.globalContainerMounts = ImmutableList.of(); + expected.app.allowedContainerTypes = ImmutableList.of(); + expected.app.defaultDockerParameters = ImmutableList.of(); + expected.taskAssigner.offerSelectorModules = ImmutableList.of(); + expected.state.taskAssignerModules = ImmutableList.of(); + expected.aop.methodInterceptorModules = ImmutableList.of(); + expected.httpSecurity.shiroRealmModule = ImmutableList.of(); + expected.preemptor.slotFinderModules = ImmutableList.of(); + expected.sla.slaProdMetrics = ImmutableList.of(); + expected.sla.slaNonProdMetrics = ImmutableList.of(); + + CliOptions parsed = CommandLine.parseOptions( + "-cluster_name=testing", + "-mesos_master_address=testing", + "-backup_dir=testing", + "-serverset_path=testing", + "-zk_endpoints=testing:42", + "-offer_order=", + "-offer_order_modules=", + "-thermos_executor_resources=", + "-global_container_mounts=", + "-allowed_container_types=", + "-default_docker_parameters=", + "-offer_selector_modules=", + "-task_assigner_modules=", + "-thrift_method_interceptor_modules=", + "-shiro_realm_modules=", + "-preemption_slot_finder_modules=", + "-sla_prod_metrics=", + "-sla_non_prod_metrics="); + + assertEqualOptions(expected, parsed); + + // Test other ways in which an empty list can be passed + // as thermos executor resources such as "" and ''. + parsed = CommandLine.parseOptions( + "-cluster_name=testing", + "-mesos_master_address=testing", + "-backup_dir=testing", + "-serverset_path=testing", + "-zk_endpoints=testing:42", + "-thermos_executor_resources=\"\""); + + assertEquals(ImmutableList.of(), parsed.executor.thermosExecutorResources); + + parsed = CommandLine.parseOptions( + "-cluster_name=testing", + "-mesos_master_address=testing", + "-backup_dir=testing", + "-serverset_path=testing", + "-zk_endpoints=testing:42", + "-thermos_executor_resources=''"); + + assertEquals(ImmutableList.of(), parsed.executor.thermosExecutorResources); + } + private static void assertEqualOptions(CliOptions expected, CliOptions actual) { List<Object> actualObjects = CommandLine.getOptionsObjects(actual); for (Object expectedOptionContainer : CommandLine.getOptionsObjects(expected)) {
