Repository: aurora Updated Branches: refs/heads/master e57993bfb -> 4ee10b047
Alter thrift wrapper generator to use default primitive values and empty collections. Bugs closed: AURORA-1476 Reviewed at https://reviews.apache.org/r/38112/ Project: http://git-wip-us.apache.org/repos/asf/aurora/repo Commit: http://git-wip-us.apache.org/repos/asf/aurora/commit/4ee10b04 Tree: http://git-wip-us.apache.org/repos/asf/aurora/tree/4ee10b04 Diff: http://git-wip-us.apache.org/repos/asf/aurora/diff/4ee10b04 Branch: refs/heads/master Commit: 4ee10b0474fcfb20c2b7a462488a490f7695e2b2 Parents: e57993b Author: Bill Farner <[email protected]> Authored: Thu Sep 10 20:49:45 2015 -0700 Committer: Bill Farner <[email protected]> Committed: Thu Sep 10 20:49:45 2015 -0700 ---------------------------------------------------------------------- .../configuration/ConfigurationManager.java | 63 +--- .../aurora/scheduler/quota/QuotaManager.java | 4 - .../scheduler/storage/StorageBackfill.java | 13 - .../thrift/SchedulerThriftInterface.java | 31 +- .../aurora/tools/java/thrift_wrapper_codegen.py | 318 ++++++++++++------- .../aurora/scheduler/app/SchedulerIT.java | 6 +- .../configuration/ConfigurationManagerTest.java | 8 - .../filter/SchedulingFilterImplTest.java | 5 +- .../aurora/scheduler/http/api/ApiBetaTest.java | 11 +- .../preemptor/PendingTaskProcessorTest.java | 21 +- .../scheduler/quota/QuotaManagerImplTest.java | 12 +- .../storage/AbstractTaskStoreTest.java | 15 + .../scheduler/storage/StorageBackfillTest.java | 10 +- .../storage/db/DbAttributeStoreTest.java | 5 +- .../storage/db/DbJobUpdateStoreTest.java | 38 ++- .../aurora/scheduler/thrift/Fixtures.java | 3 +- .../thrift/ReadOnlySchedulerImplTest.java | 40 ++- .../thrift/SchedulerThriftInterfaceTest.java | 25 +- 18 files changed, 352 insertions(+), 276 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/aurora/blob/4ee10b04/src/main/java/org/apache/aurora/scheduler/configuration/ConfigurationManager.java ---------------------------------------------------------------------- diff --git a/src/main/java/org/apache/aurora/scheduler/configuration/ConfigurationManager.java b/src/main/java/org/apache/aurora/scheduler/configuration/ConfigurationManager.java index a952797..3a2056a 100644 --- a/src/main/java/org/apache/aurora/scheduler/configuration/ConfigurationManager.java +++ b/src/main/java/org/apache/aurora/scheduler/configuration/ConfigurationManager.java @@ -19,7 +19,6 @@ import java.util.regex.Pattern; import javax.annotation.Nullable; -import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Optional; import com.google.common.base.Predicate; import com.google.common.base.Strings; @@ -27,15 +26,11 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.google.common.collect.Iterables; -import com.google.common.collect.Maps; -import com.google.common.collect.Sets; import org.apache.aurora.common.args.Arg; import org.apache.aurora.common.args.CmdLine; -import org.apache.aurora.common.base.Closure; import org.apache.aurora.gen.Container; import org.apache.aurora.gen.JobConfiguration; -import org.apache.aurora.gen.MesosContainer; import org.apache.aurora.gen.TaskConfig; import org.apache.aurora.gen.TaskConfig._Fields; import org.apache.aurora.gen.TaskConstraint; @@ -73,23 +68,6 @@ public final class ConfigurationManager { private static final int MAX_IDENTIFIER_LENGTH = 255; - private static class DefaultField implements Closure<TaskConfig> { - private final _Fields field; - private final Object defaultValue; - - DefaultField(_Fields field, Object defaultValue) { - this.field = field; - this.defaultValue = defaultValue; - } - - @Override - public void execute(TaskConfig task) { - if (!task.isSet(field)) { - task.setFieldValue(field, defaultValue); - } - } - } - private interface Validator<T> { void validate(T value) throws TaskDescriptionException; } @@ -130,19 +108,6 @@ public final class ConfigurationManager { } } - private static final Iterable<Closure<TaskConfig>> DEFAULT_FIELD_POPULATORS = - ImmutableList.of( - new DefaultField(_Fields.IS_SERVICE, false), - new DefaultField(_Fields.PRIORITY, 0), - new DefaultField(_Fields.PRODUCTION, false), - new DefaultField(_Fields.MAX_TASK_FAILURES, 1), - new DefaultField(_Fields.TASK_LINKS, Maps.newHashMap()), - new DefaultField(_Fields.REQUESTED_PORTS, Sets.newHashSet()), - new DefaultField(_Fields.CONSTRAINTS, Sets.newHashSet()), - // TODO(wfarner): Explore replacing these with thrift defaults. - new DefaultField(_Fields.CONTAINER, - Container.mesos(new MesosContainer()))); - private static final Iterable<RequiredFieldValidator<?>> REQUIRED_FIELDS_VALIDATORS = ImmutableList.of( new RequiredFieldValidator<>(_Fields.NUM_CPUS, new GreaterThan(0.0, "num_cpus")), @@ -222,10 +187,6 @@ public final class ConfigurationManager { throw new TaskDescriptionException("Job configuration must have taskConfig set."); } - if (!job.isSetInstanceCount()) { - throw new TaskDescriptionException("Job configuration does not have instanceCount set."); - } - if (job.getInstanceCount() <= 0) { throw new TaskDescriptionException("Instance count must be positive."); } @@ -366,7 +327,7 @@ public final class ConfigurationManager { "The container type " + containerType.get().toString() + " is not allowed"); } - return ITaskConfig.build(applyDefaultsIfUnset(builder)); + return ITaskConfig.build(builder); } /** @@ -379,28 +340,6 @@ public final class ConfigurationManager { return constraint -> constraint.getName().equals(name); } - /** - * Applies defaults to unset values in a task. - * @param task Task to apply defaults to. - * @return A reference to the (modified) {@code task}. - */ - public static TaskConfig applyDefaultsIfUnset(TaskConfig task) { - for (Closure<TaskConfig> populator : DEFAULT_FIELD_POPULATORS) { - populator.execute(task); - } - return task; - } - - /** - * Applies defaults to unset values in a job and its tasks. - * - * @param job Job to apply defaults to. - */ - @VisibleForTesting - public static void applyDefaultsIfUnset(JobConfiguration job) { - ConfigurationManager.applyDefaultsIfUnset(job.getTaskConfig()); - } - private static void maybeFillLinks(TaskConfig task) { if (task.getTaskLinksSize() == 0) { ImmutableMap.Builder<String, String> links = ImmutableMap.builder(); http://git-wip-us.apache.org/repos/asf/aurora/blob/4ee10b04/src/main/java/org/apache/aurora/scheduler/quota/QuotaManager.java ---------------------------------------------------------------------- diff --git a/src/main/java/org/apache/aurora/scheduler/quota/QuotaManager.java b/src/main/java/org/apache/aurora/scheduler/quota/QuotaManager.java index 49bf3c9..6fbb582 100644 --- a/src/main/java/org/apache/aurora/scheduler/quota/QuotaManager.java +++ b/src/main/java/org/apache/aurora/scheduler/quota/QuotaManager.java @@ -156,10 +156,6 @@ public interface QuotaManager { final IResourceAggregate quota, MutableStoreProvider storeProvider) throws QuotaException { - if (!quota.isSetNumCpus() || !quota.isSetRamMb() || !quota.isSetDiskMb()) { - throw new QuotaException("Missing quota specification(s) in: " + quota.toString()); - } - if (quota.getNumCpus() < 0.0 || quota.getRamMb() < 0 || quota.getDiskMb() < 0) { throw new QuotaException("Negative values in: " + quota.toString()); } http://git-wip-us.apache.org/repos/asf/aurora/blob/4ee10b04/src/main/java/org/apache/aurora/scheduler/storage/StorageBackfill.java ---------------------------------------------------------------------- diff --git a/src/main/java/org/apache/aurora/scheduler/storage/StorageBackfill.java b/src/main/java/org/apache/aurora/scheduler/storage/StorageBackfill.java index f1b167b..a0483f4 100644 --- a/src/main/java/org/apache/aurora/scheduler/storage/StorageBackfill.java +++ b/src/main/java/org/apache/aurora/scheduler/storage/StorageBackfill.java @@ -17,14 +17,12 @@ import java.util.concurrent.atomic.AtomicLong; import java.util.logging.Logger; import org.apache.aurora.common.stats.Stats; - import org.apache.aurora.gen.JobConfiguration; import org.apache.aurora.gen.JobKey; import org.apache.aurora.gen.ScheduledTask; import org.apache.aurora.gen.TaskConfig; import org.apache.aurora.scheduler.base.JobKeys; import org.apache.aurora.scheduler.base.Query; -import org.apache.aurora.scheduler.configuration.ConfigurationManager; import org.apache.aurora.scheduler.storage.Storage.MutableStoreProvider; import org.apache.aurora.scheduler.storage.TaskStore.Mutable.TaskMutation; import org.apache.aurora.scheduler.storage.entities.IJobConfiguration; @@ -50,7 +48,6 @@ public final class StorageBackfill { private static void backfillJobDefaults(CronJobStore.Mutable jobStore) { for (JobConfiguration job : IJobConfiguration.toBuildersList(jobStore.fetchJobs())) { populateJobKey(job.getTaskConfig(), BACKFILLED_JOB_CONFIG_KEYS); - ConfigurationManager.applyDefaultsIfUnset(job); jobStore.saveAcceptedJob(IJobConfiguration.build(job)); } } @@ -86,15 +83,5 @@ public final class StorageBackfill { return IScheduledTask.build(builder); } }); - - LOG.info("Performing shard uniqueness sanity check."); - storeProvider.getUnsafeTaskStore().mutateTasks(Query.unscoped(), new TaskMutation() { - @Override - public IScheduledTask apply(final IScheduledTask task) { - ScheduledTask builder = task.newBuilder(); - ConfigurationManager.applyDefaultsIfUnset(builder.getAssignedTask().getTask()); - return IScheduledTask.build(builder); - } - }); } } http://git-wip-us.apache.org/repos/asf/aurora/blob/4ee10b04/src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java ---------------------------------------------------------------------- diff --git a/src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java b/src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java index e12ad3e..d69bc39 100644 --- a/src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java +++ b/src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java @@ -49,10 +49,7 @@ import org.apache.aurora.gen.ConfigRewrite; import org.apache.aurora.gen.DrainHostsResult; import org.apache.aurora.gen.EndMaintenanceResult; import org.apache.aurora.gen.Hosts; -import org.apache.aurora.gen.InstanceConfigRewrite; -import org.apache.aurora.gen.InstanceKey; import org.apache.aurora.gen.InstanceTaskConfig; -import org.apache.aurora.gen.JobConfigRewrite; import org.apache.aurora.gen.JobConfiguration; import org.apache.aurora.gen.JobKey; import org.apache.aurora.gen.JobUpdate; @@ -110,6 +107,10 @@ import org.apache.aurora.scheduler.storage.Storage.StoreProvider; import org.apache.aurora.scheduler.storage.backup.Recovery; import org.apache.aurora.scheduler.storage.backup.StorageBackup; import org.apache.aurora.scheduler.storage.entities.IAssignedTask; +import org.apache.aurora.scheduler.storage.entities.IConfigRewrite; +import org.apache.aurora.scheduler.storage.entities.IInstanceConfigRewrite; +import org.apache.aurora.scheduler.storage.entities.IInstanceKey; +import org.apache.aurora.scheduler.storage.entities.IJobConfigRewrite; import org.apache.aurora.scheduler.storage.entities.IJobConfiguration; import org.apache.aurora.scheduler.storage.entities.IJobKey; import org.apache.aurora.scheduler.storage.entities.IJobUpdate; @@ -802,7 +803,7 @@ class SchedulerThriftInterface implements AnnotatedAuroraAdmin { List<String> errors = Lists.newArrayList(); for (ConfigRewrite command : request.getRewriteCommands()) { - Optional<String> error = rewriteConfig(command, storeProvider); + Optional<String> error = rewriteConfig(IConfigRewrite.build(command), storeProvider); if (error.isPresent()) { errors.add(error.get()); } @@ -821,13 +822,12 @@ class SchedulerThriftInterface implements AnnotatedAuroraAdmin { }); } - private Optional<String> rewriteJob(JobConfigRewrite jobRewrite, CronJobStore.Mutable jobStore) { - IJobConfiguration existingJob = IJobConfiguration.build(jobRewrite.getOldJob()); + private Optional<String> rewriteJob(IJobConfigRewrite jobRewrite, CronJobStore.Mutable jobStore) { + IJobConfiguration existingJob = jobRewrite.getOldJob(); IJobConfiguration rewrittenJob; Optional<String> error = Optional.absent(); try { - rewrittenJob = ConfigurationManager.validateAndPopulate( - IJobConfiguration.build(jobRewrite.getRewrittenJob())); + rewrittenJob = ConfigurationManager.validateAndPopulate(jobRewrite.getRewrittenJob()); } catch (TaskDescriptionException e) { // We could add an error here, but this is probably a hint of something wrong in // the client that's causing a bad configuration to be applied. @@ -856,13 +856,13 @@ class SchedulerThriftInterface implements AnnotatedAuroraAdmin { } private Optional<String> rewriteInstance( - InstanceConfigRewrite instanceRewrite, + IInstanceConfigRewrite instanceRewrite, MutableStoreProvider storeProvider) { - InstanceKey instanceKey = instanceRewrite.getInstanceKey(); + IInstanceKey instanceKey = instanceRewrite.getInstanceKey(); Optional<String> error = Optional.absent(); Iterable<IScheduledTask> tasks = storeProvider.getTaskStore().fetchTasks( - Query.instanceScoped(IJobKey.build(instanceKey.getJobKey()), + Query.instanceScoped(instanceKey.getJobKey(), instanceKey.getInstanceId()) .active()); Optional<IAssignedTask> task = @@ -870,9 +870,8 @@ class SchedulerThriftInterface implements AnnotatedAuroraAdmin { .transform(IScheduledTask::getAssignedTask); if (task.isPresent()) { - if (task.get().getTask().newBuilder().equals(instanceRewrite.getOldTask())) { - ITaskConfig newConfiguration = ITaskConfig.build( - ConfigurationManager.applyDefaultsIfUnset(instanceRewrite.getRewrittenTask())); + if (task.get().getTask().equals(instanceRewrite.getOldTask())) { + ITaskConfig newConfiguration = instanceRewrite.getRewrittenTask(); boolean changed = storeProvider.getUnsafeTaskStore().unsafeModifyInPlace( task.get().getTaskId(), newConfiguration); if (!changed) { @@ -889,7 +888,7 @@ class SchedulerThriftInterface implements AnnotatedAuroraAdmin { } private Optional<String> rewriteConfig( - ConfigRewrite command, + IConfigRewrite command, MutableStoreProvider storeProvider) { Optional<String> error; @@ -1133,7 +1132,7 @@ class SchedulerThriftInterface implements AnnotatedAuroraAdmin { return invalidRequest(INVALID_MIN_WAIT_TO_RUNNING); } - if (settings.isSetBlockIfNoPulsesAfterMs() && settings.getBlockIfNoPulsesAfterMs() <= 0) { + if (settings.getBlockIfNoPulsesAfterMs() < 0) { return invalidRequest(INVALID_PULSE_TIMEOUT); } http://git-wip-us.apache.org/repos/asf/aurora/blob/4ee10b04/src/main/python/apache/aurora/tools/java/thrift_wrapper_codegen.py ---------------------------------------------------------------------- diff --git a/src/main/python/apache/aurora/tools/java/thrift_wrapper_codegen.py b/src/main/python/apache/aurora/tools/java/thrift_wrapper_codegen.py index b5f2bc9..49cfa9f 100644 --- a/src/main/python/apache/aurora/tools/java/thrift_wrapper_codegen.py +++ b/src/main/python/apache/aurora/tools/java/thrift_wrapper_codegen.py @@ -32,6 +32,9 @@ class Type(object): def absolute_name(self): return '%s.%s' % (self.package, self.name) if self.package else self.name + def codegen_name(self): + return self.name + def __str__(self): return '%s (%smutable)' % (self.absolute_name(), 'im' if self.immutable else '') @@ -53,8 +56,8 @@ class ParameterizedType(Type): def param_names(self): def name(t): - if isinstance(t, StructType): - return t.name if t.kind == 'enum' else t.codegen_name + if isinstance(t, StructType) and not t.immutable: + return t.codegen_name() elif isinstance(t, PrimitiveType): return t.boxed_name else: @@ -67,10 +70,12 @@ class StructType(Type): def __init__(self, name, package, kind, fields): Type.__init__(self, name, package, kind == 'enum') - self.codegen_name = 'I%s' % name self.kind = kind self.fields = fields + def codegen_name(self): + return 'I%s' % self.name + def __str__(self): return '%s %s { %s }' % (self.kind, self.name, ', '.join(map(str, self.fields))) @@ -91,10 +96,13 @@ class Field(object): self.ttype = ttype self.name = name + def capitalized_name(self): + return self.name[:1].capitalize() + self.name[1:] + def accessor_method(self): return '%s%s' % ( 'is' if self.ttype.name == 'boolean' else 'get', - self.name[:1].capitalize() + self.name[1:]) + self.capitalized_name()) def isset_method(self): return 'isSet%s' % (self.name[0].upper() + self.name[1:]) @@ -107,32 +115,45 @@ FIELD_TEMPLATE = ''' public %(type)s %(fn_name)s() { return %(field)s; }''' - -# Template string for a method to access an immutable field. -IMMUTABLE_FIELD_TEMPLATE = ''' public %(type)s %(fn_name)s() { - return wrapped.%(fn_name)s(); +UNION_FIELD_TEMPLATE = ''' public %(type)s %(fn_name)s() { + if (getSetField() == %(enum_value)s) { + return (%(type)s) value; + } else { + throw new RuntimeException("Cannot get field '%(enum_value)s' " + + "because union is currently set to " + getSetField()); + } }''' +UNION_SWITCH_CASE = '''case %(case)s: + %(body)s''' + +UNION_FIELD_SWITCH = '''switch (getSetField()) { + %(cases)s + default: + throw new RuntimeException("Unrecognized field " + getSetField()); + }''' -STRUCT_DECLARATION = '''private final %(type)s %(field)s;''' -STRUCT_ASSIGNMENT = '''this.%(field)s = !wrapped.%(isset)s() - ? null - : %(type)s.buildNoCopy(wrapped.%(fn_name)s());''' +SIMPLE_ASSIGNMENT = 'this.%(field)s = wrapped.%(fn_name)s();' + +FIELD_DECLARATION = '''private final %(type)s %(field)s;''' +STRUCT_ASSIGNMENT = '''this.%(field)s = wrapped.%(isset)s() + ? %(type)s.build(wrapped.%(fn_name)s()) + : null;''' IMMUTABLE_COLLECTION_DECLARATION = ( '''private final Immutable%(collection)s<%(params)s> %(field)s;''') -IMMUTABLE_COLLECTION_ASSIGNMENT = '''this.%(field)s = !wrapped.%(isset)s() - ? Immutable%(collection)s.of() - : Immutable%(collection)s.copyOf(wrapped.%(fn_name)s());''' +IMMUTABLE_COLLECTION_ASSIGNMENT = '''this.%(field)s = wrapped.%(isset)s() + ? Immutable%(collection)s.copyOf(wrapped.%(fn_name)s()) + : Immutable%(collection)s.of();''' # Template string for assignment for a collection field containing a struct. -STRUCT_COLLECTION_FIELD_ASSIGNMENT = '''this.%(field)s = !wrapped.%(isset)s() - ? Immutable%(collection)s.<%(params)s>of() - : FluentIterable.from(wrapped.%(fn_name)s()) +STRUCT_COLLECTION_FIELD_ASSIGNMENT = '''this.%(field)s = wrapped.%(isset)s() + ? FluentIterable.from(wrapped.%(fn_name)s()) .transform(%(params)s::build) - .to%(collection)s();''' + .to%(collection)s() + : Immutable%(collection)s.<%(params)s>of();''' PACKAGE_NAME = 'org.apache.aurora.scheduler.storage.entities' @@ -147,25 +168,23 @@ CLASS_TEMPLATE = '''package %(package)s; * This code is auto-generated, and should not be directly modified. */ public final class %(name)s { - private final %(wrapped)s wrapped; private int cachedHashCode = 0; %(fields)s - private %(name)s(%(wrapped)s wrapped) { - this.wrapped = Objects.requireNonNull(wrapped);%(assignments)s - } - - static %(name)s buildNoCopy(%(wrapped)s wrapped) { - return new %(name)s(wrapped); + private %(name)s(%(wrapped)s wrapped) {%(assignments)s } public static %(name)s build(%(wrapped)s wrapped) { - return buildNoCopy(wrapped.deepCopy()); + return new %(name)s(wrapped); } public static ImmutableList<%(wrapped)s> toBuildersList(Iterable<%(name)s> w) { return FluentIterable.from(w).transform(%(name)s::newBuilder).toList(); } + static List<%(wrapped)s> toMutableBuildersList(Iterable<%(name)s> w) { + return Lists.newArrayList(Iterables.transform(w, %(name)s::newBuilder)); + } + public static ImmutableList<%(name)s> listFromBuilders(Iterable<%(wrapped)s> b) { return FluentIterable.from(b).transform(%(name)s::build).toList(); } @@ -174,12 +193,16 @@ public final class %(name)s { return FluentIterable.from(w).transform(%(name)s::newBuilder).toSet(); } + static Set<%(wrapped)s> toMutableBuildersSet(Iterable<%(name)s> w) { + return Sets.newHashSet(Iterables.transform(w, %(name)s::newBuilder)); + } + public static ImmutableSet<%(name)s> setFromBuilders(Iterable<%(wrapped)s> b) { return FluentIterable.from(b).transform(%(name)s::build).toSet(); } public %(wrapped)s newBuilder() { - return wrapped.deepCopy(); + %(copy_constructor)s } %(accessors)s @@ -190,7 +213,7 @@ public final class %(name)s { return false; } %(name)s other = (%(name)s) o; - return wrapped.equals(other.wrapped); + return %(equals)s; } @Override @@ -200,14 +223,15 @@ public final class %(name)s { // computing the value, which is apparently favorable to constant // synchronization overhead. if (cachedHashCode == 0) { - cachedHashCode = wrapped.hashCode(); + cachedHashCode = Objects.hash(%(hashcode)s); } return cachedHashCode; } @Override public String toString() { - return wrapped.toString(); + return MoreObjects.toStringHelper(this)%(to_string)s + .toString(); } }''' @@ -220,18 +244,25 @@ class GeneratedCode(object): self._accessors = [] self._fields = [] self._assignments = [] + self.to_string = 'unset' + self.hash_code = 'unset' + self.equals = 'unset' + self.builder = 'unset' + self.copy_constructor = 'unset' def add_import(self, import_class): self._imports.add(import_class) - def add_assignment(self, field, assignment): + def add_field(self, field): self._fields.append(field) + + def add_assignment(self, assignment): self._assignments.append(assignment) def add_accessor(self, accessor_method): self._accessors.append(accessor_method) - def dump(self, f): + def dump(self, out_file): remaining_imports = list(self._imports) import_groups = [] def remove_by_prefix(prefix): @@ -258,7 +289,11 @@ class GeneratedCode(object): 'accessors': '\n\n'.join(self._accessors), 'fields': (' ' + '\n '.join(self._fields) + '\n') if self._fields else '', 'assignments': ('\n ' + '\n '.join(self._assignments)) if self._assignments else '', - }, file=f) + 'to_string': self.to_string, + 'equals': self.equals, + 'hashcode': self.hash_code, + 'copy_constructor': self.copy_constructor, + }, file=out_file) # A namespace declaration, e.g.: @@ -443,87 +478,154 @@ def parse_services(service_defs): return services +def to_upper_snake_case(s): + return re.sub('([A-Z])', '_\\1', s).upper() + + +def generate_union_field(code, struct, field): + field_enum_value = '%s._Fields.%s' % (struct.name, to_upper_snake_case(field.name)) + code.add_accessor(FIELD_TEMPLATE % {'type': 'boolean', + 'fn_name': field.isset_method(), + 'field': 'setField == %s' % field_enum_value}) + code.add_accessor(UNION_FIELD_TEMPLATE % {'type': field.ttype.codegen_name(), + 'fn_name': field.accessor_method(), + 'enum_value': field_enum_value}) + + +def generate_struct_field(code, struct, field, builder_calls): + field_type = field.ttype.codegen_name() + assignment = SIMPLE_ASSIGNMENT + assignment_args = { + 'field': field.name, + 'fn_name': field.accessor_method() + } + builder_assignment = field.name + + if field.ttype.immutable: + code.add_accessor(FIELD_TEMPLATE % {'type': field.ttype.name, + 'fn_name': field.accessor_method(), + 'field': field.name}) + else: + if isinstance(field.ttype, ParameterizedType): + # Add imports for any referenced enum types. This is not necessary for other + # types since they are either primitives or struct types, which will be in + # the same package. + for param_type in field.ttype.params: + if isinstance(param_type, StructType) and param_type.kind == 'enum': + code.add_import(param_type.absolute_name()) + + field_type = 'Immutable%s<%s>' % (field.ttype.name, field.ttype.param_names()) + code.add_accessor(FIELD_TEMPLATE % {'type': field_type, + 'fn_name': field.accessor_method(), + 'field': field.name}) + + if isinstance(field.ttype, StructType): + if field.ttype.kind == 'enum': + field_type = field.ttype.name + code.add_import(field.ttype.absolute_name()) + + if not field.ttype.immutable: + assignment = STRUCT_ASSIGNMENT + assignment_args = { + 'field': field.name, + 'fn_name': field.accessor_method(), + 'isset': field.isset_method(), + 'type': field.ttype.codegen_name(), + } + builder_assignment = '%s.newBuilder()' % field.name + elif isinstance(field.ttype, ParameterizedType): + # Add necessary imports, supporting only List, Map, Set. + assert field.ttype.name in ['List', 'Map', 'Set'], 'Unrecognized type %s' % field.ttype.name + code.add_import('com.google.common.collect.Immutable%s' % field.ttype.name) + + params = field.ttype.params + if all([p.immutable for p in params]): + # All parameter types are immutable. + assignment = IMMUTABLE_COLLECTION_ASSIGNMENT + elif len(params) == 1: + # Only one non-immutable parameter. + # Assumes the parameter type is a struct and our code generator + # will make a compatible wrapper class and constructor. + assignment = STRUCT_COLLECTION_FIELD_ASSIGNMENT + builder_assignment = '%s.toMutableBuilders%s(%s)' % (params[0].codegen_name(), field.ttype.name, field.name) + else: + assert False, 'Unable to codegen accessor field for %s' % field.name + assignment_args = {'collection': field.ttype.name, + 'field': field.name, + 'fn_name': field.accessor_method(), + 'isset': field.isset_method(), + 'params': field.ttype.param_names()} + + code.add_field(FIELD_DECLARATION % {'field': field.name, 'type': field_type }) + + nullable = field.ttype.name == 'String' or not isinstance(field.ttype, PrimitiveType) + if nullable: + code.add_accessor(FIELD_TEMPLATE % {'type': 'boolean', + 'fn_name': field.isset_method(), + 'field': '%s != null' % field.name}) + builder_calls.append('.set%s(%s == null ? null : %s)' % (field.capitalized_name(), field.name, builder_assignment)) + else: + builder_calls.append('.set%s(%s)' % (field.capitalized_name(), builder_assignment)) + code.add_assignment(assignment % assignment_args) + def generate_java(struct): - code = GeneratedCode(struct.codegen_name, struct.name) + code = GeneratedCode(struct.codegen_name(), struct.name) code.add_import('java.util.Objects') + code.add_import('java.util.List') + code.add_import('java.util.Set') + code.add_import('com.google.common.base.MoreObjects') code.add_import('com.google.common.collect.ImmutableList') code.add_import('com.google.common.collect.ImmutableSet') code.add_import('com.google.common.collect.FluentIterable') + code.add_import('com.google.common.collect.Iterables') + code.add_import('com.google.common.collect.Lists') + code.add_import('com.google.common.collect.Sets') code.add_import(struct.absolute_name()) if struct.kind == 'union': - code.add_accessor(IMMUTABLE_FIELD_TEMPLATE - % {'type': '%s._Fields' % struct.name, 'fn_name': 'getSetField'}) - - # Accessor for each field. - for field in struct.fields: - code.add_accessor(IMMUTABLE_FIELD_TEMPLATE - % {'type': 'boolean', - 'fn_name': field.isset_method()}) - if field.ttype.immutable: - code.add_accessor(IMMUTABLE_FIELD_TEMPLATE % {'type': field.ttype.name, - 'fn_name': field.accessor_method()}) - elif not struct.kind == 'union': - if isinstance(field.ttype, StructType): - return_type = field.ttype.codegen_name - elif isinstance(field.ttype, ParameterizedType): - # Add imports for any referenced enum types. This is not necessary for other - # types since they are either primitives or struct types, which will be in - # the same package. - for param_type in field.ttype.params: - if isinstance(param_type, StructType) and param_type.kind == 'enum': - code.add_import(param_type.absolute_name()) - - return_type = 'Immutable%s<%s>' % (field.ttype.name, field.ttype.param_names()) - else: - return_type = field.ttype.name - code.add_accessor(FIELD_TEMPLATE % {'type': return_type, - 'fn_name': field.accessor_method(), - 'field': field.name}) - - if isinstance(field.ttype, StructType): - if field.ttype.kind == 'enum': - code.add_import(field.ttype.absolute_name()) - - if field.ttype.immutable: - # Direct accessor was already added. - pass - elif struct.kind == 'union': - copy_field = '%s.build(wrapped.%s())' % (field.ttype.codegen_name, - field.accessor_method()) - code.add_accessor(FIELD_TEMPLATE % {'type': field.ttype.codegen_name, - 'fn_name': field.accessor_method(), - 'field': copy_field}) - else: - args = { - 'field': field.name, - 'fn_name': field.accessor_method(), - 'isset': field.isset_method(), - 'type': field.ttype.codegen_name, - } - code.add_assignment(STRUCT_DECLARATION % args, STRUCT_ASSIGNMENT % args) - elif isinstance(field.ttype, ParameterizedType): - # Add necessary imports, supporting only List, Map, Set. - assert field.ttype.name in ['List', 'Map', 'Set'], 'Unrecognized type %s' % field.ttype.name - code.add_import('com.google.common.collect.Immutable%s' % field.ttype.name) - - params = field.ttype.params - if all([p.immutable for p in params]): - # All parameter types are immutable. - assignment = IMMUTABLE_COLLECTION_ASSIGNMENT - elif len(params) == 1: - # Only one non-immutable parameter. - # Assumes the parameter type is a struct and our code generator - # will make a compatible wrapper class and constructor. - assignment = STRUCT_COLLECTION_FIELD_ASSIGNMENT - else: - assert False, 'Unable to codegen accessor field for %s' % field.name - args = {'collection': field.ttype.name, - 'field': field.name, - 'fn_name': field.accessor_method(), - 'isset': field.isset_method(), - 'params': field.ttype.param_names()} - code.add_assignment(IMMUTABLE_COLLECTION_DECLARATION % args, assignment % args) + assign_cases = [] + copy_cases = [] + for field in struct.fields: + generate_union_field(code, struct, field) + + assign_case_body = 'value = %(codegen_name)s.build(wrapped.%(accessor_method)s());\nbreak;' % { + 'codegen_name': field.ttype.codegen_name(), + 'accessor_method': field.accessor_method()} + assign_cases.append(UNION_SWITCH_CASE % {'case': to_upper_snake_case(field.name), + 'body': assign_case_body}) + + copy_case_body = 'return new %s(setField, %s().newBuilder());' % (struct.name, field.accessor_method()) + copy_cases.append(UNION_SWITCH_CASE % {'case': to_upper_snake_case(field.name), + 'body': copy_case_body}) + + set_field_type = '%s._Fields' % struct.name + code.add_accessor(FIELD_TEMPLATE % {'type': set_field_type, 'fn_name': 'getSetField', 'field': 'setField'}) + code.add_field(FIELD_DECLARATION % {'field': 'setField', 'type': set_field_type}) + code.add_assignment(SIMPLE_ASSIGNMENT % {'field': 'setField', + 'fn_name': 'getSetField'}) + code.add_field(FIELD_DECLARATION % {'field': 'value', 'type': 'Object'}) + code.add_assignment(UNION_FIELD_SWITCH % {'cases': '\n '.join(assign_cases)}) + + code.copy_constructor = UNION_FIELD_SWITCH % {'cases': '\n '.join(copy_cases)} + + code.to_string = '.add("setField", setField).add("value", value)' + code.equals = 'Objects.equals(setField, other.setField) && Objects.equals(value, other.value)' + code.hash_code = 'setField, value' + else: + builder_calls = [] + for field in struct.fields: + generate_struct_field(code, struct, field, builder_calls) + + field_names = [f.name for f in struct.fields] + code.copy_constructor = 'return new %s()%s;' % (struct.name, '\n ' + '\n '.join(builder_calls)) + code.to_string = '\n ' + '\n '.join(['.add("%s", %s)' % (f, f) for f in field_names]) + code.equals = '\n && '.join(['Objects.equals(%s, other.%s)' % (f, f) for f in field_names]) + code.hash_code = '\n ' + ',\n '.join([f for f in field_names]) + + # Special case for structs with no fields. + if not struct.fields: + code.equals = 'true' + return code if __name__ == '__main__': @@ -559,7 +661,7 @@ if __name__ == '__main__': # Skip generation for enums, since they are immutable. if struct.kind == 'enum': continue - gen_file = os.path.join(package_dir, '%s.java' % struct.codegen_name) + gen_file = os.path.join(package_dir, '%s.java' % struct.codegen_name()) log('Generating %s' % gen_file) with open(gen_file, 'w') as f: code = generate_java(struct) http://git-wip-us.apache.org/repos/asf/aurora/blob/4ee10b04/src/test/java/org/apache/aurora/scheduler/app/SchedulerIT.java ---------------------------------------------------------------------- diff --git a/src/test/java/org/apache/aurora/scheduler/app/SchedulerIT.java b/src/test/java/org/apache/aurora/scheduler/app/SchedulerIT.java index b2ec13f..c58ce71 100644 --- a/src/test/java/org/apache/aurora/scheduler/app/SchedulerIT.java +++ b/src/test/java/org/apache/aurora/scheduler/app/SchedulerIT.java @@ -77,7 +77,6 @@ import org.apache.aurora.gen.storage.storageConstants; import org.apache.aurora.scheduler.ResourceSlot; import org.apache.aurora.scheduler.async.AsyncModule.AsyncExecutor; import org.apache.aurora.scheduler.async.FlushableWorkQueue; -import org.apache.aurora.scheduler.configuration.ConfigurationManager; import org.apache.aurora.scheduler.log.Log; import org.apache.aurora.scheduler.log.Log.Entry; import org.apache.aurora.scheduler.log.Log.Position; @@ -306,7 +305,7 @@ public class SchedulerIT extends BaseZooKeeperTest { } private static ScheduledTask makeTask(String id, ScheduleStatus status) { - ScheduledTask scheduledTask = new ScheduledTask() + return new ScheduledTask() .setStatus(status) .setTaskEvents(ImmutableList.of(new TaskEvent(100, status))) .setAssignedTask(new AssignedTask() @@ -318,9 +317,6 @@ public class SchedulerIT extends BaseZooKeeperTest { .setEnvironment("test") .setExecutorConfig(new org.apache.aurora.gen.ExecutorConfig("AuroraExecutor", "")) .setOwner(new Identity("role-" + id, "user-" + id)))); - // Apply defaults here so that we can expect the same task that will be written to the stream. - ConfigurationManager.applyDefaultsIfUnset(scheduledTask.getAssignedTask().getTask()); - return scheduledTask; } @Test http://git-wip-us.apache.org/repos/asf/aurora/blob/4ee10b04/src/test/java/org/apache/aurora/scheduler/configuration/ConfigurationManagerTest.java ---------------------------------------------------------------------- diff --git a/src/test/java/org/apache/aurora/scheduler/configuration/ConfigurationManagerTest.java b/src/test/java/org/apache/aurora/scheduler/configuration/ConfigurationManagerTest.java index f3b62cc..c6a4ac5 100644 --- a/src/test/java/org/apache/aurora/scheduler/configuration/ConfigurationManagerTest.java +++ b/src/test/java/org/apache/aurora/scheduler/configuration/ConfigurationManagerTest.java @@ -103,14 +103,6 @@ public class ConfigurationManagerTest { } } - @Test - public void testApplyDefaultsIfUnsetUnsanitized() { - JobConfiguration copy = UNSANITIZED_JOB_CONFIGURATION.deepCopy(); - - ConfigurationManager.applyDefaultsIfUnset(copy); - assertTrue(copy.isSetKey()); - } - @Test(expected = TaskDescriptionException.class) public void testBadContainerConfig() throws TaskDescriptionException { TaskConfig taskConfig = CONFIG_WITH_CONTAINER.deepCopy(); http://git-wip-us.apache.org/repos/asf/aurora/blob/4ee10b04/src/test/java/org/apache/aurora/scheduler/filter/SchedulingFilterImplTest.java ---------------------------------------------------------------------- diff --git a/src/test/java/org/apache/aurora/scheduler/filter/SchedulingFilterImplTest.java b/src/test/java/org/apache/aurora/scheduler/filter/SchedulingFilterImplTest.java index 5d8bd1b..3b1766d 100644 --- a/src/test/java/org/apache/aurora/scheduler/filter/SchedulingFilterImplTest.java +++ b/src/test/java/org/apache/aurora/scheduler/filter/SchedulingFilterImplTest.java @@ -35,7 +35,6 @@ import org.apache.aurora.gen.TaskConstraint; import org.apache.aurora.gen.ValueConstraint; import org.apache.aurora.scheduler.ResourceSlot; import org.apache.aurora.scheduler.Resources; -import org.apache.aurora.scheduler.configuration.ConfigurationManager; import org.apache.aurora.scheduler.filter.SchedulingFilter.ResourceRequest; import org.apache.aurora.scheduler.filter.SchedulingFilter.UnusedResource; import org.apache.aurora.scheduler.filter.SchedulingFilter.Veto; @@ -626,13 +625,13 @@ public class SchedulingFilterImplTest extends EasyMockTest { long ramMb, long diskMb) { - return ITaskConfig.build(ConfigurationManager.applyDefaultsIfUnset(new TaskConfig() + return ITaskConfig.build(new TaskConfig() .setOwner(owner) .setJobName(jobName) .setNumCpus(cpus) .setRamMb(ramMb) .setDiskMb(diskMb) - .setExecutorConfig(new ExecutorConfig("aurora", "config")))); + .setExecutorConfig(new ExecutorConfig("aurora", "config"))); } private ITaskConfig makeTask(int cpus, long ramMb, long diskMb) { http://git-wip-us.apache.org/repos/asf/aurora/blob/4ee10b04/src/test/java/org/apache/aurora/scheduler/http/api/ApiBetaTest.java ---------------------------------------------------------------------- diff --git a/src/test/java/org/apache/aurora/scheduler/http/api/ApiBetaTest.java b/src/test/java/org/apache/aurora/scheduler/http/api/ApiBetaTest.java index 08e1284..8af7c46 100644 --- a/src/test/java/org/apache/aurora/scheduler/http/api/ApiBetaTest.java +++ b/src/test/java/org/apache/aurora/scheduler/http/api/ApiBetaTest.java @@ -53,6 +53,7 @@ import org.apache.aurora.gen.TaskConstraint; import org.apache.aurora.gen.TaskQuery; import org.apache.aurora.scheduler.http.JettyServerModuleTest; import org.apache.aurora.scheduler.storage.entities.IJobConfiguration; +import org.apache.aurora.scheduler.storage.entities.IResponse; import org.apache.aurora.scheduler.storage.entities.ITaskConfig; import org.apache.aurora.scheduler.thrift.aop.AnnotatedAuroraAdmin; import org.junit.Before; @@ -60,6 +61,8 @@ import org.junit.Test; import static org.apache.aurora.gen.ResponseCode.OK; import static org.apache.aurora.gen.ScheduleStatus.RUNNING; +import static org.easymock.EasyMock.anyObject; +import static org.easymock.EasyMock.eq; import static org.easymock.EasyMock.expect; import static org.junit.Assert.assertEquals; @@ -118,7 +121,7 @@ public class ApiBetaTest extends JettyServerModuleTest { .setResponseCode(OK); JobConfiguration job = JOB_CONFIG.newBuilder(); - expect(thrift.createJob(job, lock, session)).andReturn(response); + expect(thrift.createJob(anyObject(), eq(lock), eq(session))).andReturn(response); replayAndStart(); @@ -127,7 +130,7 @@ public class ApiBetaTest extends JettyServerModuleTest { ImmutableMap.of("description", job, "lock", lock, "session", session), MediaType.APPLICATION_JSON) .post(Response.class); - assertEquals(response, actualResponse); + assertEquals(IResponse.build(response), IResponse.build(actualResponse)); } @Test @@ -164,7 +167,7 @@ public class ApiBetaTest extends JettyServerModuleTest { Response actualResponse = getRequestBuilder("/apibeta/getJobSummary") .entity(ImmutableMap.of("role", "roleA"), MediaType.APPLICATION_JSON) .post(Response.class); - assertEquals(response, actualResponse); + assertEquals(IResponse.build(response), IResponse.build(actualResponse)); } @Test @@ -190,7 +193,7 @@ public class ApiBetaTest extends JettyServerModuleTest { Response actualResponse = getRequestBuilder("/apibeta/getTasksStatus") .entity(ImmutableMap.of("query", query), MediaType.APPLICATION_JSON) .post(Response.class); - assertEquals(response, actualResponse); + assertEquals(IResponse.build(response), IResponse.build(actualResponse)); } @Test http://git-wip-us.apache.org/repos/asf/aurora/blob/4ee10b04/src/test/java/org/apache/aurora/scheduler/preemptor/PendingTaskProcessorTest.java ---------------------------------------------------------------------- diff --git a/src/test/java/org/apache/aurora/scheduler/preemptor/PendingTaskProcessorTest.java b/src/test/java/org/apache/aurora/scheduler/preemptor/PendingTaskProcessorTest.java index 9213b88..49002d0 100644 --- a/src/test/java/org/apache/aurora/scheduler/preemptor/PendingTaskProcessorTest.java +++ b/src/test/java/org/apache/aurora/scheduler/preemptor/PendingTaskProcessorTest.java @@ -28,18 +28,19 @@ import org.apache.aurora.common.testing.easymock.EasyMockTest; import org.apache.aurora.common.util.testing.FakeClock; import org.apache.aurora.gen.AssignedTask; import org.apache.aurora.gen.HostAttributes; -import org.apache.aurora.gen.JobKey; import org.apache.aurora.gen.MaintenanceMode; import org.apache.aurora.gen.ScheduledTask; import org.apache.aurora.gen.TaskConfig; import org.apache.aurora.gen.TaskEvent; import org.apache.aurora.scheduler.HostOffer; +import org.apache.aurora.scheduler.base.JobKeys; import org.apache.aurora.scheduler.base.Query; import org.apache.aurora.scheduler.base.TaskGroupKey; import org.apache.aurora.scheduler.filter.AttributeAggregate; import org.apache.aurora.scheduler.offers.OfferManager; import org.apache.aurora.scheduler.stats.CachedCounters; import org.apache.aurora.scheduler.storage.entities.IHostAttributes; +import org.apache.aurora.scheduler.storage.entities.IJobKey; import org.apache.aurora.scheduler.storage.entities.IScheduledTask; import org.apache.aurora.scheduler.storage.entities.ITaskConfig; import org.apache.aurora.scheduler.storage.testing.StorageTestUtil; @@ -58,13 +59,14 @@ import static org.easymock.EasyMock.eq; import static org.easymock.EasyMock.expect; import static org.easymock.EasyMock.expectLastCall; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; public class PendingTaskProcessorTest extends EasyMockTest { private static final String CACHE_STAT = "cache_size"; private static final String SLAVE_ID_1 = "slave_id_1"; private static final String SLAVE_ID_2 = "slave_id_2"; - private static final JobKey JOB_A = new JobKey("role_a", "env", "job_a"); - private static final JobKey JOB_B = new JobKey("role_b", "env", "job_b"); + private static final IJobKey JOB_A = JobKeys.from("role_a", "env", "job_a"); + private static final IJobKey JOB_B = JobKeys.from("role_b", "env", "job_b"); private static final IScheduledTask TASK_A = makeTask(JOB_A, SLAVE_ID_1, "id1"); private static final IScheduledTask TASK_B = makeTask(JOB_B, SLAVE_ID_2, "id2"); private static final PreemptionProposal SLOT_A = createPreemptionProposal(TASK_A, SLAVE_ID_1); @@ -194,7 +196,12 @@ public class PendingTaskProcessorTest extends EasyMockTest { assertEquals(1L, statsProvider.getLongValue(TASK_PROCESSOR_RUN_NAME)); assertEquals(3L, statsProvider.getLongValue(attemptsStatName(true))); assertEquals(2L, statsProvider.getLongValue(slotSearchStatName(true, true))); - assertEquals(2L, statsProvider.getLongValue(slotSearchStatName(false, true))); + + // TODO(wfarner): This test depends on the iteration order of a hash set (the set containing + // task groups), and as a result this stat could be 1 or 2 depending on which group is + // evaluated first. + assertTrue(ImmutableSet.of(1L, 2L).contains( + statsProvider.getLongValue(slotSearchStatName(false, true)))); assertEquals(2L, statsProvider.getLongValue(CACHE_STAT)); } @@ -258,7 +265,7 @@ public class PendingTaskProcessorTest extends EasyMockTest { slaveId); } - private static IScheduledTask makeTask(JobKey key, String taskId) { + private static IScheduledTask makeTask(IJobKey key, String taskId) { return makeTask(key, null, taskId); } @@ -266,7 +273,7 @@ public class PendingTaskProcessorTest extends EasyMockTest { return TaskGroupKey.from(task.getAssignedTask().getTask()); } - private static IScheduledTask makeTask(JobKey key, @Nullable String slaveId, String taskId) { + private static IScheduledTask makeTask(IJobKey key, @Nullable String slaveId, String taskId) { ScheduledTask task = new ScheduledTask() .setAssignedTask(new AssignedTask() .setSlaveId(slaveId) @@ -274,7 +281,7 @@ public class PendingTaskProcessorTest extends EasyMockTest { .setTask(new TaskConfig() .setPriority(1) .setProduction(true) - .setJob(key))); + .setJob(key.newBuilder()))); task.addToTaskEvents(new TaskEvent(0, PENDING)); return IScheduledTask.build(task); } http://git-wip-us.apache.org/repos/asf/aurora/blob/4ee10b04/src/test/java/org/apache/aurora/scheduler/quota/QuotaManagerImplTest.java ---------------------------------------------------------------------- diff --git a/src/test/java/org/apache/aurora/scheduler/quota/QuotaManagerImplTest.java b/src/test/java/org/apache/aurora/scheduler/quota/QuotaManagerImplTest.java index 83a8abe..e68fc1d 100644 --- a/src/test/java/org/apache/aurora/scheduler/quota/QuotaManagerImplTest.java +++ b/src/test/java/org/apache/aurora/scheduler/quota/QuotaManagerImplTest.java @@ -37,6 +37,7 @@ import org.apache.aurora.gen.ScheduledTask; import org.apache.aurora.gen.TaskConfig; import org.apache.aurora.gen.TaskConstraint; import org.apache.aurora.gen.ValueConstraint; +import org.apache.aurora.scheduler.ResourceAggregates; import org.apache.aurora.scheduler.base.JobKeys; import org.apache.aurora.scheduler.base.Query; import org.apache.aurora.scheduler.quota.QuotaManager.QuotaException; @@ -716,8 +717,15 @@ public class QuotaManagerImplTest extends EasyMockTest { storageUtil.mutableStoreProvider); } - @Test(expected = QuotaException.class) - public void testSaveQuotaFailsMissingSpecs() throws Exception { + @Test + public void testRemoveQuota() throws Exception { + expectNoJobUpdates(); + expectNoCronJobs(); + expectNoTasks(); + expectQuota(IResourceAggregate.build(new ResourceAggregate(1, 1, 1))); + + storageUtil.quotaStore.saveQuota(ROLE, ResourceAggregates.EMPTY); + control.replay(); quotaManager.saveQuota( ROLE, http://git-wip-us.apache.org/repos/asf/aurora/blob/4ee10b04/src/test/java/org/apache/aurora/scheduler/storage/AbstractTaskStoreTest.java ---------------------------------------------------------------------- diff --git a/src/test/java/org/apache/aurora/scheduler/storage/AbstractTaskStoreTest.java b/src/test/java/org/apache/aurora/scheduler/storage/AbstractTaskStoreTest.java index 295974a..fa1d1cd 100644 --- a/src/test/java/org/apache/aurora/scheduler/storage/AbstractTaskStoreTest.java +++ b/src/test/java/org/apache/aurora/scheduler/storage/AbstractTaskStoreTest.java @@ -632,6 +632,21 @@ public abstract class AbstractTaskStoreTest extends TearDownTestCase { }); } + @Test + public void testNullVsEmptyRelations() throws Exception { + // Test for regression of AURORA-1476. + + ITaskConfig nullMetadata = + ITaskConfig.build(TaskTestUtil.makeConfig(TaskTestUtil.JOB).newBuilder().setMetadata(null)); + + IScheduledTask a = TaskTestUtil.makeTask("a", nullMetadata); + IScheduledTask b = TaskTestUtil.makeTask("a", nullMetadata); + IScheduledTask c = TaskTestUtil.makeTask("a", nullMetadata); + saveTasks(a); + saveTasks(b); + saveTasks(c); + } + private void assertStoreContents(IScheduledTask... tasks) { assertQueryResults(Query.unscoped(), tasks); } http://git-wip-us.apache.org/repos/asf/aurora/blob/4ee10b04/src/test/java/org/apache/aurora/scheduler/storage/StorageBackfillTest.java ---------------------------------------------------------------------- diff --git a/src/test/java/org/apache/aurora/scheduler/storage/StorageBackfillTest.java b/src/test/java/org/apache/aurora/scheduler/storage/StorageBackfillTest.java index 7ccc273..2e3ac5a 100644 --- a/src/test/java/org/apache/aurora/scheduler/storage/StorageBackfillTest.java +++ b/src/test/java/org/apache/aurora/scheduler/storage/StorageBackfillTest.java @@ -22,7 +22,6 @@ import org.apache.aurora.gen.MesosContainer; import org.apache.aurora.gen.ScheduledTask; import org.apache.aurora.scheduler.base.Query; import org.apache.aurora.scheduler.base.TaskTestUtil; -import org.apache.aurora.scheduler.configuration.ConfigurationManager; import org.apache.aurora.scheduler.storage.db.DbUtil; import org.apache.aurora.scheduler.storage.entities.IScheduledTask; import org.junit.Before; @@ -47,11 +46,8 @@ public class StorageBackfillTest { } @Test - public void testBackfillTask() throws Exception { - ScheduledTask task = TASK.newBuilder(); - ConfigurationManager.applyDefaultsIfUnset(task.getAssignedTask().getTask()); - - final Set<IScheduledTask> backfilledTasks = ImmutableSet.of(IScheduledTask.build(task)); + public void testBackfillTask() { + final Set<IScheduledTask> backfilledTasks = ImmutableSet.of(TASK); storage.write(new Storage.MutateWork.NoResult.Quiet() { @Override public void execute(Storage.MutableStoreProvider storeProvider) { @@ -62,7 +58,7 @@ public class StorageBackfillTest { backfill(); assertEquals( - ImmutableSet.of(IScheduledTask.build(task)), + ImmutableSet.of(TASK), Storage.Util.fetchTasks(storage, Query.unscoped())); } http://git-wip-us.apache.org/repos/asf/aurora/blob/4ee10b04/src/test/java/org/apache/aurora/scheduler/storage/db/DbAttributeStoreTest.java ---------------------------------------------------------------------- diff --git a/src/test/java/org/apache/aurora/scheduler/storage/db/DbAttributeStoreTest.java b/src/test/java/org/apache/aurora/scheduler/storage/db/DbAttributeStoreTest.java index 0e0a847..20c9858 100644 --- a/src/test/java/org/apache/aurora/scheduler/storage/db/DbAttributeStoreTest.java +++ b/src/test/java/org/apache/aurora/scheduler/storage/db/DbAttributeStoreTest.java @@ -118,12 +118,13 @@ public class DbAttributeStoreTest { insert(IHostAttributes.build(noMode)); } - @Test(expected = IllegalArgumentException.class) - public void testSaveAttributesNotSet() { + @Test + public void testSaveAttributesEmpty() { HostAttributes attributes = HOST_A_ATTRS.newBuilder(); attributes.unsetAttributes(); insert(IHostAttributes.build(attributes)); + assertEquals(Optional.of(IHostAttributes.build(attributes)), read(HOST_A)); } @Test http://git-wip-us.apache.org/repos/asf/aurora/blob/4ee10b04/src/test/java/org/apache/aurora/scheduler/storage/db/DbJobUpdateStoreTest.java ---------------------------------------------------------------------- diff --git a/src/test/java/org/apache/aurora/scheduler/storage/db/DbJobUpdateStoreTest.java b/src/test/java/org/apache/aurora/scheduler/storage/db/DbJobUpdateStoreTest.java index 3e78c09..4a09693 100644 --- a/src/test/java/org/apache/aurora/scheduler/storage/db/DbJobUpdateStoreTest.java +++ b/src/test/java/org/apache/aurora/scheduler/storage/db/DbJobUpdateStoreTest.java @@ -17,6 +17,9 @@ package org.apache.aurora.scheduler.storage.db; import java.util.List; import java.util.Map; import java.util.Set; +import java.util.function.Consumer; +import java.util.stream.Collectors; +import java.util.stream.Stream; import com.google.common.base.Optional; import com.google.common.collect.ImmutableList; @@ -108,12 +111,37 @@ public class DbJobUpdateStoreTest { truncateUpdates(); } + private static IJobUpdate makeFullyPopulatedUpdate(IJobUpdateKey key) { + JobUpdate builder = makeJobUpdate(key).newBuilder(); + JobUpdateInstructions instructions = builder.getInstructions(); + Stream.of( + instructions.getInitialState().stream() + .map(InstanceTaskConfig::getInstances) + .flatMap(Set::stream) + .collect(Collectors.toSet()), + instructions.getDesiredState().getInstances(), + instructions.getSettings().getUpdateOnlyTheseInstances()) + .flatMap(Set::stream) + .forEach(new Consumer<Range>() { + @Override + public void accept(Range range) { + if (range.getFirst() == 0) { + range.setFirst(1); + } + if (range.getLast() == 0) { + range.setLast(1); + } + } + }); + return IJobUpdate.build(builder); + } + @Test public void testSaveJobUpdates() { IJobUpdateKey updateId1 = makeKey(JobKeys.from("role", "env", "name1"), "u1"); IJobUpdateKey updateId2 = makeKey(JobKeys.from("role", "env", "name2"), "u2"); - IJobUpdate update1 = makeJobUpdate(updateId1); + IJobUpdate update1 = makeFullyPopulatedUpdate(updateId1); IJobUpdate update2 = makeJobUpdate(updateId2); assertEquals(Optional.absent(), getUpdate(updateId1)); @@ -1003,7 +1031,7 @@ public class DbJobUpdateStoreTest { .setInstanceEvents(IJobInstanceUpdateEvent.toBuildersList(instanceEvents))); } - private IJobUpdateSummary makeSummary(IJobUpdateKey key, String user) { + private static IJobUpdateSummary makeSummary(IJobUpdateKey key, String user) { return IJobUpdateSummary.build(new JobUpdateSummary() .setKey(key.newBuilder()) .setUser(user)); @@ -1030,17 +1058,17 @@ public class DbJobUpdateStoreTest { return IJobUpdate.build(makeJobUpdate().newBuilder().setSummary(summary.newBuilder())); } - private IJobUpdate makeJobUpdate(IJobUpdateKey key) { + private static IJobUpdate makeJobUpdate(IJobUpdateKey key) { return IJobUpdate.build(makeJobUpdate().newBuilder() .setSummary(makeSummary(key, "user").newBuilder())); } - private IJobUpdate makeJobUpdate() { + private static IJobUpdate makeJobUpdate() { return IJobUpdate.build(new JobUpdate() .setInstructions(makeJobUpdateInstructions().newBuilder())); } - private IJobUpdateInstructions makeJobUpdateInstructions() { + private static IJobUpdateInstructions makeJobUpdateInstructions() { TaskConfig config = TaskTestUtil.makeConfig(JOB).newBuilder(); return IJobUpdateInstructions.build(new JobUpdateInstructions() .setDesiredState(new InstanceTaskConfig() http://git-wip-us.apache.org/repos/asf/aurora/blob/4ee10b04/src/test/java/org/apache/aurora/scheduler/thrift/Fixtures.java ---------------------------------------------------------------------- diff --git a/src/test/java/org/apache/aurora/scheduler/thrift/Fixtures.java b/src/test/java/org/apache/aurora/scheduler/thrift/Fixtures.java index b058719..adcc02b 100644 --- a/src/test/java/org/apache/aurora/scheduler/thrift/Fixtures.java +++ b/src/test/java/org/apache/aurora/scheduler/thrift/Fixtures.java @@ -50,6 +50,7 @@ import org.apache.aurora.scheduler.storage.entities.IJobUpdateKey; import org.apache.aurora.scheduler.storage.entities.ILock; import org.apache.aurora.scheduler.storage.entities.ILockKey; import org.apache.aurora.scheduler.storage.entities.IResourceAggregate; +import org.apache.aurora.scheduler.storage.entities.IResult; import org.apache.aurora.scheduler.storage.entities.IScheduledTask; import static org.apache.aurora.gen.ResponseCode.OK; @@ -140,7 +141,7 @@ final class Fixtures { } static Response okResponse(Result result) { - return response(OK, Optional.of(result)); + return response(OK, Optional.of(IResult.build(result).newBuilder())); } static JobConfiguration makeProdJob() { http://git-wip-us.apache.org/repos/asf/aurora/blob/4ee10b04/src/test/java/org/apache/aurora/scheduler/thrift/ReadOnlySchedulerImplTest.java ---------------------------------------------------------------------- diff --git a/src/test/java/org/apache/aurora/scheduler/thrift/ReadOnlySchedulerImplTest.java b/src/test/java/org/apache/aurora/scheduler/thrift/ReadOnlySchedulerImplTest.java index 37c8129..64cbd5b 100644 --- a/src/test/java/org/apache/aurora/scheduler/thrift/ReadOnlySchedulerImplTest.java +++ b/src/test/java/org/apache/aurora/scheduler/thrift/ReadOnlySchedulerImplTest.java @@ -68,11 +68,13 @@ import org.apache.aurora.scheduler.metadata.NearestFit; import org.apache.aurora.scheduler.quota.QuotaInfo; import org.apache.aurora.scheduler.quota.QuotaManager; import org.apache.aurora.scheduler.state.LockManager; +import org.apache.aurora.scheduler.storage.entities.IConfigSummaryResult; import org.apache.aurora.scheduler.storage.entities.IJobConfiguration; import org.apache.aurora.scheduler.storage.entities.IJobKey; import org.apache.aurora.scheduler.storage.entities.IJobUpdateDetails; import org.apache.aurora.scheduler.storage.entities.IJobUpdateQuery; import org.apache.aurora.scheduler.storage.entities.IJobUpdateSummary; +import org.apache.aurora.scheduler.storage.entities.IResponse; import org.apache.aurora.scheduler.storage.entities.IScheduledTask; import org.apache.aurora.scheduler.storage.entities.ITaskConfig; import org.apache.aurora.scheduler.storage.testing.StorageTestUtil; @@ -211,12 +213,9 @@ public class ReadOnlySchedulerImplTest extends EasyMockTest { assertEquals(jobSummaryResponse(ownedCronJobSummaryOnly), thrift.getJobSummary(ROLE)); Response jobSummaryResponse = thrift.getJobSummary(ROLE); - assertEquals(jobSummaryResponse(ownedImmedieteJobSummaryOnly), jobSummaryResponse); - assertEquals(ownedImmediateTaskInfo, - Iterables.getOnlyElement( - jobSummaryResponse.getResult().getJobSummaryResult().getSummaries()) - .getJob() - .getTaskConfig()); + assertEquals( + jobSummaryResponse(ownedImmedieteJobSummaryOnly), + IResponse.build(jobSummaryResponse).newBuilder()); assertEquals(jobSummaryResponse(ImmutableSet.of()), thrift.getJobSummary(ROLE)); @@ -372,7 +371,10 @@ public class ReadOnlySchedulerImplTest extends EasyMockTest { Set<JobConfiguration> allJobs = ImmutableSet.<JobConfiguration>builder().addAll(crons).add(immediateJob).build(); - assertEquals(allJobs, thrift.getJobs(null).getResult().getGetJobsResult().getConfigs()); + assertEquals( + IJobConfiguration.setFromBuilders(allJobs), + IJobConfiguration.setFromBuilders( + thrift.getJobs(null).getResult().getGetJobsResult().getConfigs())); } @Test @@ -432,24 +434,30 @@ public class ReadOnlySchedulerImplTest extends EasyMockTest { control.replay(); - assertEquals(ownedCronJob, Iterables.getOnlyElement(thrift.getJobs(ROLE) + assertJobsEqual(ownedCronJob, Iterables.getOnlyElement(thrift.getJobs(ROLE) .getResult().getGetJobsResult().getConfigs())); - assertEquals(ownedCronJob, Iterables.getOnlyElement(thrift.getJobs(ROLE) + assertJobsEqual(ownedCronJob, Iterables.getOnlyElement(thrift.getJobs(ROLE) .getResult().getGetJobsResult().getConfigs())); Set<JobConfiguration> queryResult3 = thrift.getJobs(ROLE).getResult().getGetJobsResult().getConfigs(); - assertEquals(ownedImmediateJob, Iterables.getOnlyElement(queryResult3)); - assertEquals(ownedImmediateTaskInfo, Iterables.getOnlyElement(queryResult3).getTaskConfig()); + assertJobsEqual(ownedImmediateJob, Iterables.getOnlyElement(queryResult3)); + assertEquals( + ITaskConfig.build(ownedImmediateTaskInfo), + ITaskConfig.build(Iterables.getOnlyElement(queryResult3).getTaskConfig())); assertTrue(thrift.getJobs(ROLE) .getResult().getGetJobsResult().getConfigs().isEmpty()); - assertEquals(ownedCronJob, Iterables.getOnlyElement(thrift.getJobs(ROLE) + assertJobsEqual(ownedCronJob, Iterables.getOnlyElement(thrift.getJobs(ROLE) .getResult().getGetJobsResult().getConfigs())); } + private static void assertJobsEqual(JobConfiguration expected, JobConfiguration actual) { + assertEquals(IJobConfiguration.build(expected), IJobConfiguration.build(actual)); + } + @Test public void testGetTasksStatusPagination() throws Exception { Iterable<IScheduledTask> tasks = makeDefaultScheduledTasks(10); @@ -526,7 +534,9 @@ public class ReadOnlySchedulerImplTest extends EasyMockTest { control.replay(); Response response = assertOkResponse(thrift.getConfigSummary(key.newBuilder())); - assertEquals(expected, response.getResult().getConfigSummaryResult()); + assertEquals( + IConfigSummaryResult.build(expected), + IConfigSummaryResult.build(response.getResult().getConfigSummaryResult())); } @Test @@ -576,8 +586,8 @@ public class ReadOnlySchedulerImplTest extends EasyMockTest { Response response = assertOkResponse(thrift.getJobUpdateDetails(UPDATE_KEY.newBuilder())); assertEquals( - details, - response.getResult().getGetJobUpdateDetailsResult().getDetails()); + IJobUpdateDetails.build(details), + IJobUpdateDetails.build(response.getResult().getGetJobUpdateDetailsResult().getDetails())); } private static List<JobUpdateSummary> createJobUpdateSummaries(int count) { http://git-wip-us.apache.org/repos/asf/aurora/blob/4ee10b04/src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java ---------------------------------------------------------------------- diff --git a/src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java b/src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java index 4685aa1..ac89618 100644 --- a/src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java +++ b/src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java @@ -470,7 +470,7 @@ public class SchedulerThriftInterfaceTest extends EasyMockTest { @Test public void testCreateHomogeneousJobNoInstances() throws Exception { JobConfiguration job = makeJob(); - job.unsetInstanceCount(); + job.setInstanceCount(0); expectAuth(ROLE, true); control.replay(); @@ -481,7 +481,7 @@ public class SchedulerThriftInterfaceTest extends EasyMockTest { @Test public void testCreateJobNegativeInstanceCount() throws Exception { JobConfiguration job = makeJob(); - job.setInstanceCount(0 - 1); + job.setInstanceCount(-1); expectAuth(ROLE, true); control.replay(); @@ -496,9 +496,9 @@ public class SchedulerThriftInterfaceTest extends EasyMockTest { control.replay(); TaskConfig task = productionTask(); - task.unsetNumCpus(); - task.unsetRamMb(); - task.unsetDiskMb(); + task.setNumCpus(0); + task.setRamMb(0); + task.setDiskMb(0); assertResponse(INVALID_REQUEST, thrift.createJob(makeJob(task), null, SESSION)); } @@ -562,7 +562,7 @@ public class SchedulerThriftInterfaceTest extends EasyMockTest { .setRequestedPorts(ImmutableSet.of()) .setTaskLinks(ImmutableMap.of()) .setConstraints(ImmutableSet.of()) - .setMaxTaskFailures(1) + .setMaxTaskFailures(0) .setEnvironment("devel"); lockManager.validateIfLocked(LOCK_KEY, java.util.Optional.empty()); @@ -622,10 +622,10 @@ public class SchedulerThriftInterfaceTest extends EasyMockTest { return IScheduledTask.build(new ScheduledTask() .setAssignedTask(new AssignedTask() .setInstanceId(instanceId) - .setTask(ConfigurationManager.applyDefaultsIfUnset(populatedTask() + .setTask(populatedTask() .setRamMb(5) .setIsService(true) - .setExecutorConfig(new ExecutorConfig().setData(executorData)))))); + .setExecutorConfig(new ExecutorConfig().setData(executorData))))); } private IScheduledTask buildScheduledTask() { @@ -1351,10 +1351,7 @@ public class SchedulerThriftInterfaceTest extends EasyMockTest { storageUtil.expectTaskFetch( Query.instanceScoped(IInstanceKey.build(instanceKey)).active(), storedTask); - expect(storageUtil.taskStore.unsafeModifyInPlace( - taskId, - ITaskConfig.build(ConfigurationManager.applyDefaultsIfUnset(modifiedConfig.newBuilder())))) - .andReturn(true); + expect(storageUtil.taskStore.unsafeModifyInPlace(taskId, modifiedConfig)) .andReturn(true); control.replay(); @@ -1383,7 +1380,7 @@ public class SchedulerThriftInterfaceTest extends EasyMockTest { Query.instanceScoped(IInstanceKey.build(instanceKey)).active(), task); expect(storageUtil.taskStore.unsafeModifyInPlace( taskId, - ITaskConfig.build(ConfigurationManager.applyDefaultsIfUnset(config.deepCopy())))) + ITaskConfig.build(config.deepCopy()))) .andReturn(false); control.replay(); @@ -2458,7 +2455,7 @@ public class SchedulerThriftInterfaceTest extends EasyMockTest { return new JobUpdateRequest() .setInstanceCount(6) .setSettings(buildJobUpdateSettings()) - .setTaskConfig(ConfigurationManager.applyDefaultsIfUnset(config)); + .setTaskConfig(config); } private static JobUpdateSettings buildJobUpdateSettings() {
