From f895ea77d82e986ce80ecb036859abf18b232481 Mon Sep 17 00:00:00 2001 From: Scott Wegner Date: Tue, 5 Apr 2016 11:49:26 -0700 Subject: [PATCH 01/13] Publish DisplayData for PipelineOptions. --- .../DataflowPipelineTranslatorTest.java | 5 +- .../beam/sdk/options/PipelineOptionSpec.java | 89 ++++++ .../beam/sdk/options/PipelineOptions.java | 4 +- .../sdk/options/PipelineOptionsFactory.java | 128 ++++----- .../sdk/options/PipelineOptionsReflector.java | 112 ++++++++ .../sdk/options/ProxyInvocationHandler.java | 223 +++++++++++++-- .../sdk/transforms/display/DisplayData.java | 26 +- .../options/PipelineOptionsReflectorTest.java | 197 +++++++++++++ .../options/ProxyInvocationHandlerTest.java | 268 +++++++++++++++++- .../transforms/display/DisplayDataTest.java | 25 ++ 10 files changed, 980 insertions(+), 97 deletions(-) create mode 100644 sdks/java/core/src/main/java/org/apache/beam/sdk/options/PipelineOptionSpec.java create mode 100644 sdks/java/core/src/main/java/org/apache/beam/sdk/options/PipelineOptionsReflector.java create mode 100644 sdks/java/core/src/test/java/org/apache/beam/sdk/options/PipelineOptionsReflectorTest.java diff --git a/runners/google-cloud-dataflow-java/src/test/java/org/apache/beam/sdk/runners/DataflowPipelineTranslatorTest.java b/runners/google-cloud-dataflow-java/src/test/java/org/apache/beam/sdk/runners/DataflowPipelineTranslatorTest.java index a62f55042bf9..27c0acc65917 100644 --- a/runners/google-cloud-dataflow-java/src/test/java/org/apache/beam/sdk/runners/DataflowPipelineTranslatorTest.java +++ b/runners/google-cloud-dataflow-java/src/test/java/org/apache/beam/sdk/runners/DataflowPipelineTranslatorTest.java @@ -204,8 +204,9 @@ public void testSettingOfSdkPipelineOptions() throws IOException { settings.put("numberOfWorkerHarnessThreads", 0); settings.put("experiments", null); - assertEquals(ImmutableMap.of("options", settings), - job.getEnvironment().getSdkPipelineOptions()); + Map sdkPipelineOptions = job.getEnvironment().getSdkPipelineOptions(); + assertThat(sdkPipelineOptions, hasKey("options")); + assertEquals(settings, sdkPipelineOptions.get("options")); } @Test diff --git a/sdks/java/core/src/main/java/org/apache/beam/sdk/options/PipelineOptionSpec.java b/sdks/java/core/src/main/java/org/apache/beam/sdk/options/PipelineOptionSpec.java new file mode 100644 index 000000000000..71f9d462bbf7 --- /dev/null +++ b/sdks/java/core/src/main/java/org/apache/beam/sdk/options/PipelineOptionSpec.java @@ -0,0 +1,89 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you 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.beam.sdk.options; + +import com.google.common.base.MoreObjects; +import com.google.common.base.Objects; + +import java.lang.reflect.Method; + +/** + * For internal use. Specification for an option defined in a {@link PipelineOptions} interface. + */ +class PipelineOptionSpec { + private final Class clazz; + private final String name; + private final Method getter; + + static PipelineOptionSpec of(Class clazz, String name, Method getter) { + return new PipelineOptionSpec(clazz, name, getter); + } + + private PipelineOptionSpec(Class clazz, String name, Method getter) { + this.clazz = clazz; + this.name = name; + this.getter = getter; + } + + /** + * The {@link PipelineOptions} interface which defines this {@link PipelineOptionSpec}. + */ + Class getDefiningInterface() { + return clazz; + } + + /** + * Name of the property. + */ + String getName() { + return name; + } + + /** + * The getter method for this property. + */ + Method getGetterMethod() { + return getter; + } + + @Override + public String toString() { + return MoreObjects.toStringHelper(this) + .add("definingInterface", getDefiningInterface()) + .add("name", getName()) + .add("getterMethod", getGetterMethod()) + .toString(); + } + + @Override + public int hashCode() { + return Objects.hashCode(getDefiningInterface(), getName(), getGetterMethod()); + } + + @Override + public boolean equals(Object obj) { + if (!(obj instanceof PipelineOptionSpec)) { + return false; + } + + PipelineOptionSpec that = (PipelineOptionSpec) obj; + return Objects.equal(this.getDefiningInterface(), that.getDefiningInterface()) + && Objects.equal(this.getName(), that.getName()) + && Objects.equal(this.getGetterMethod(), that.getGetterMethod()); + } +} diff --git a/sdks/java/core/src/main/java/org/apache/beam/sdk/options/PipelineOptions.java b/sdks/java/core/src/main/java/org/apache/beam/sdk/options/PipelineOptions.java index 17cf5b38d1a4..a2f38edd36c6 100644 --- a/sdks/java/core/src/main/java/org/apache/beam/sdk/options/PipelineOptions.java +++ b/sdks/java/core/src/main/java/org/apache/beam/sdk/options/PipelineOptions.java @@ -25,7 +25,7 @@ import org.apache.beam.sdk.runners.PipelineRunner; import org.apache.beam.sdk.transforms.DoFn; import org.apache.beam.sdk.transforms.DoFn.Context; - +import org.apache.beam.sdk.transforms.display.HasDisplayData; import com.google.auto.service.AutoService; import com.fasterxml.jackson.annotation.JsonIgnore; @@ -194,7 +194,7 @@ @JsonSerialize(using = Serializer.class) @JsonDeserialize(using = Deserializer.class) @ThreadSafe -public interface PipelineOptions { +public interface PipelineOptions extends HasDisplayData { /** * Transforms this object into an object of type {@code } saving each property * that has been manipulated. {@code } must extend {@link PipelineOptions}. diff --git a/sdks/java/core/src/main/java/org/apache/beam/sdk/options/PipelineOptionsFactory.java b/sdks/java/core/src/main/java/org/apache/beam/sdk/options/PipelineOptionsFactory.java index 87ac05ead37c..5fc7312d8ef3 100644 --- a/sdks/java/core/src/main/java/org/apache/beam/sdk/options/PipelineOptionsFactory.java +++ b/sdks/java/core/src/main/java/org/apache/beam/sdk/options/PipelineOptionsFactory.java @@ -22,9 +22,10 @@ import org.apache.beam.sdk.options.Validation.Required; import org.apache.beam.sdk.runners.PipelineRunner; import org.apache.beam.sdk.runners.PipelineRunnerRegistrar; +import org.apache.beam.sdk.transforms.display.DisplayData; import org.apache.beam.sdk.util.StringUtils; import org.apache.beam.sdk.util.common.ReflectHelpers; - +import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Function; import com.google.common.base.Joiner; import com.google.common.base.Optional; @@ -32,7 +33,6 @@ import com.google.common.base.Predicate; import com.google.common.base.Strings; import com.google.common.base.Throwables; -import com.google.common.collect.ArrayListMultimap; import com.google.common.collect.Collections2; import com.google.common.collect.FluentIterable; import com.google.common.collect.ImmutableListMultimap; @@ -43,8 +43,11 @@ import com.google.common.collect.ListMultimap; import com.google.common.collect.Lists; import com.google.common.collect.Maps; +import com.google.common.collect.Ordering; +import com.google.common.collect.RowSortedTable; import com.google.common.collect.Sets; import com.google.common.collect.SortedSetMultimap; +import com.google.common.collect.TreeBasedTable; import com.google.common.collect.TreeMultimap; import com.fasterxml.jackson.annotation.JsonIgnore; @@ -77,6 +80,7 @@ import java.util.Set; import java.util.SortedMap; import java.util.SortedSet; +import java.util.TreeMap; import java.util.TreeSet; import javax.annotation.Nullable; @@ -444,6 +448,7 @@ Class getProxyClass() { @SuppressWarnings("rawtypes") private static final Class[] EMPTY_CLASS_ARRAY = new Class[0]; private static final ObjectMapper MAPPER = new ObjectMapper(); + private static final ClassLoader CLASS_LOADER; private static final Map>> SUPPORTED_PIPELINE_RUNNERS; /** Classes that are used as the boundary in the stack trace to find the callers class name. */ @@ -510,7 +515,7 @@ static ClassLoader findClassLoader() { throw new ExceptionInInitializerError(e); } - ClassLoader classLoader = findClassLoader(); + CLASS_LOADER = findClassLoader(); // Store the list of all available pipeline runners. ImmutableMap.Builder>> builder = @@ -518,25 +523,14 @@ static ClassLoader findClassLoader() { Set pipelineRunnerRegistrars = Sets.newTreeSet(ObjectsClassComparator.INSTANCE); pipelineRunnerRegistrars.addAll( - Lists.newArrayList(ServiceLoader.load(PipelineRunnerRegistrar.class, classLoader))); + Lists.newArrayList(ServiceLoader.load(PipelineRunnerRegistrar.class, CLASS_LOADER))); for (PipelineRunnerRegistrar registrar : pipelineRunnerRegistrars) { for (Class> klass : registrar.getPipelineRunners()) { builder.put(klass.getSimpleName(), klass); } } SUPPORTED_PIPELINE_RUNNERS = builder.build(); - - // Load and register the list of all classes that extend PipelineOptions. - register(PipelineOptions.class); - Set pipelineOptionsRegistrars = - Sets.newTreeSet(ObjectsClassComparator.INSTANCE); - pipelineOptionsRegistrars.addAll( - Lists.newArrayList(ServiceLoader.load(PipelineOptionsRegistrar.class, classLoader))); - for (PipelineOptionsRegistrar registrar : pipelineOptionsRegistrars) { - for (Class klass : registrar.getPipelineOptions()) { - register(klass); - } - } + initializeRegistry(); } /** @@ -565,6 +559,33 @@ public static synchronized void register(Class iface) REGISTERED_OPTIONS.add(iface); } + /** + * Resets the set of interfaces registered with this factory to the default state. + * + * @see PipelineOptionsFactory#register(Class) + */ + @VisibleForTesting + static synchronized void resetRegistry() { + REGISTERED_OPTIONS.clear(); + initializeRegistry(); + } + + /** + * Load and register the list of all classes that extend PipelineOptions. + */ + private static void initializeRegistry() { + register(PipelineOptions.class); + Set pipelineOptionsRegistrars = + Sets.newTreeSet(ObjectsClassComparator.INSTANCE); + pipelineOptionsRegistrars.addAll( + Lists.newArrayList(ServiceLoader.load(PipelineOptionsRegistrar.class, CLASS_LOADER))); + for (PipelineOptionsRegistrar registrar : pipelineOptionsRegistrars) { + for (Class klass : registrar.getPipelineOptions()) { + register(klass); + } + } + } + /** * Validates that the interface conforms to the following: *
    @@ -674,32 +695,20 @@ public static void printHelp(PrintStream out, Class i Preconditions.checkNotNull(iface); validateWellFormed(iface, REGISTERED_OPTIONS); - Iterable methods = - Iterables.filter( - ReflectHelpers.getClosureOfMethodsOnInterface(iface), NOT_SYNTHETIC_PREDICATE); - ListMultimap, Method> ifaceToMethods = ArrayListMultimap.create(); - for (Method method : methods) { - // Process only methods that are not marked as hidden. - if (method.getAnnotation(Hidden.class) == null) { - ifaceToMethods.put(method.getDeclaringClass(), method); - } + Set properties = + PipelineOptionsReflector.getOptionSpecs(iface); + + RowSortedTable, String, Method> ifacePropGetterTable = TreeBasedTable.create( + ClassNameComparator.INSTANCE, Ordering.natural()); + for (PipelineOptionSpec prop : properties) { + ifacePropGetterTable.put(prop.getDefiningInterface(), prop.getName(), prop.getGetterMethod()); } - SortedSet> ifaces = new TreeSet<>(ClassNameComparator.INSTANCE); - // Keep interfaces that are not marked as hidden. - ifaces.addAll(Collections2.filter(ifaceToMethods.keySet(), new Predicate>() { - @Override - public boolean apply(Class input) { - return input.getAnnotation(Hidden.class) == null; - } - })); - for (Class currentIface : ifaces) { - Map propertyNamesToGetters = - getPropertyNamesToGetters(ifaceToMethods.get(currentIface)); - // Don't output anything if there are no defined options - if (propertyNamesToGetters.isEmpty()) { - continue; - } + for (Map.Entry, Map> ifaceToPropertyMap : + ifacePropGetterTable.rowMap().entrySet()) { + Class currentIface = ifaceToPropertyMap.getKey(); + Map propertyNamesToGetters = ifaceToPropertyMap.getValue(); + SortedSetMultimap requiredGroupNameToProperties = getRequiredGroupNamesToProperties(propertyNamesToGetters); @@ -838,15 +847,21 @@ static List getPropertyDescriptors( *

    TODO: Swap back to using Introspector once the proxy class issue with AppEngine is * resolved. */ - private static List getPropertyDescriptors(Class beanClass) + private static List getPropertyDescriptors( + Class beanClass) throws IntrospectionException { // The sorting is important to make this method stable. SortedSet methods = Sets.newTreeSet(MethodComparator.INSTANCE); methods.addAll( Collections2.filter(Arrays.asList(beanClass.getMethods()), NOT_SYNTHETIC_PREDICATE)); - SortedMap propertyNamesToGetters = getPropertyNamesToGetters(methods); - List descriptors = Lists.newArrayList(); + SortedMap propertyNamesToGetters = new TreeMap<>(); + for (Map.Entry entry : + PipelineOptionsReflector.getPropertyNamesToGetters(methods).entries()) { + propertyNamesToGetters.put(entry.getKey(), entry.getValue()); + } + + List descriptors = Lists.newArrayList(); List mismatches = new ArrayList<>(); /* * Add all the getter/setter pairs to the list of descriptors removing the getter once @@ -918,28 +933,6 @@ private static void throwForTypeMismatches(List mismatches) { } } - /** - * Returns a map of the property name to the getter method it represents. - * If there are duplicate methods with the same bean name, then it is indeterminate - * as to which method will be returned. - */ - private static SortedMap getPropertyNamesToGetters(Iterable methods) { - SortedMap propertyNamesToGetters = Maps.newTreeMap(); - for (Method method : methods) { - String methodName = method.getName(); - if ((!methodName.startsWith("get") - && !methodName.startsWith("is")) - || method.getParameterTypes().length != 0 - || method.getReturnType() == void.class) { - continue; - } - String propertyName = Introspector.decapitalize( - methodName.startsWith("is") ? methodName.substring(2) : methodName.substring(3)); - propertyNamesToGetters.put(propertyName, method); - } - return propertyNamesToGetters; - } - /** * Returns a map of required groups of arguments to the properties that satisfy the requirement. */ @@ -981,21 +974,22 @@ private static SortedSetMultimap getRequiredGroupNamesToProperti */ private static List validateClass(Class iface, Set> validatedPipelineOptionsInterfaces, - Class klass) throws IntrospectionException { + Class klass) throws IntrospectionException { Set methods = Sets.newHashSet(IGNORED_METHODS); - // Ignore static methods, "equals", "hashCode", "toString" and "as" on the generated class. // Ignore synthetic methods for (Method method : klass.getMethods()) { if (Modifier.isStatic(method.getModifiers()) || method.isSynthetic()) { methods.add(method); } } + // Ignore standard infrastructure methods on the generated class. try { methods.add(klass.getMethod("equals", Object.class)); methods.add(klass.getMethod("hashCode")); methods.add(klass.getMethod("toString")); methods.add(klass.getMethod("as", Class.class)); methods.add(klass.getMethod("cloneAs", Class.class)); + methods.add(klass.getMethod("populateDisplayData", DisplayData.Builder.class)); } catch (NoSuchMethodException | SecurityException e) { throw Throwables.propagate(e); } diff --git a/sdks/java/core/src/main/java/org/apache/beam/sdk/options/PipelineOptionsReflector.java b/sdks/java/core/src/main/java/org/apache/beam/sdk/options/PipelineOptionsReflector.java new file mode 100644 index 000000000000..815de82585f0 --- /dev/null +++ b/sdks/java/core/src/main/java/org/apache/beam/sdk/options/PipelineOptionsReflector.java @@ -0,0 +1,112 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you 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.beam.sdk.options; + +import org.apache.beam.sdk.util.common.ReflectHelpers; +import com.google.common.collect.HashMultimap; +import com.google.common.collect.ImmutableSet; +import com.google.common.collect.Multimap; + +import java.beans.Introspector; +import java.lang.reflect.Method; +import java.util.Map; +import java.util.Set; + +/** + * Utilities to reflect over {@link PipelineOptions}. + */ +class PipelineOptionsReflector { + private PipelineOptionsReflector() {} + + /** + * Retrieve metadata for the full set of pipeline options visible within the type hierarchy + * of a single {@link PipelineOptions} interface. + * + * @see PipelineOptionsReflector#getOptionSpecs(Iterable) + */ + static Set getOptionSpecs(Class optionsInterface) { + Iterable methods = ReflectHelpers.getClosureOfMethodsOnInterface(optionsInterface); + Multimap propsToGetters = getPropertyNamesToGetters(methods); + + ImmutableSet.Builder setBuilder = ImmutableSet.builder(); + for (Map.Entry propAndGetter : propsToGetters.entries()) { + String prop = propAndGetter.getKey(); + Method getter = propAndGetter.getValue(); + + @SuppressWarnings("unchecked") + Class declaringClass = + (Class) getter.getDeclaringClass(); + + if (!PipelineOptions.class.isAssignableFrom(declaringClass)) { + continue; + } + + if (declaringClass.isAnnotationPresent(Hidden.class)) { + continue; + } + + setBuilder.add(PipelineOptionSpec.of(declaringClass, prop, getter)); + } + + return setBuilder.build(); + } + + /** + * Retrieve metadata for the full set of pipeline options visible within the type hierarchy + * closure of the set of input interfaces. An option is "visible" if: + * + *

      + *
    • The option is defined within the interface hierarchy closure of the input + * {@link PipelineOptions}.
      • + *
    • The defining interface is not marked {@link Hidden}.
      • + *
    + */ + static Set getOptionSpecs( + Iterable> optionsInterfaces) { + ImmutableSet.Builder setBuilder = ImmutableSet.builder(); + for (Class optionsInterface : optionsInterfaces) { + setBuilder.addAll(getOptionSpecs(optionsInterface)); + } + + return setBuilder.build(); + } + + /** + * Extract pipeline options and their respective getter methods from a series of + * {@link Method methods}. A single pipeline option may appear in many methods. + * + * @return A mapping of option name to the input methods which declare it. + */ + static Multimap getPropertyNamesToGetters(Iterable methods) { + Multimap propertyNamesToGetters = HashMultimap.create(); + for (Method method : methods) { + String methodName = method.getName(); + if ((!methodName.startsWith("get") + && !methodName.startsWith("is")) + || method.getParameterTypes().length != 0 + || method.getReturnType() == void.class) { + continue; + } + String propertyName = Introspector.decapitalize( + methodName.startsWith("is") ? methodName.substring(2) : methodName.substring(3)); + propertyNamesToGetters.put(propertyName, method); + } + return propertyNamesToGetters; + } + +} diff --git a/sdks/java/core/src/main/java/org/apache/beam/sdk/options/ProxyInvocationHandler.java b/sdks/java/core/src/main/java/org/apache/beam/sdk/options/ProxyInvocationHandler.java index e2816250bf7d..139a31a99bff 100644 --- a/sdks/java/core/src/main/java/org/apache/beam/sdk/options/ProxyInvocationHandler.java +++ b/sdks/java/core/src/main/java/org/apache/beam/sdk/options/ProxyInvocationHandler.java @@ -19,6 +19,8 @@ import org.apache.beam.sdk.options.PipelineOptionsFactory.JsonIgnorePredicate; import org.apache.beam.sdk.options.PipelineOptionsFactory.Registration; +import org.apache.beam.sdk.transforms.display.DisplayData; +import org.apache.beam.sdk.transforms.display.HasDisplayData; import org.apache.beam.sdk.util.InstanceBuilder; import org.apache.beam.sdk.util.common.ReflectHelpers; @@ -27,8 +29,11 @@ import com.google.common.base.Preconditions; import com.google.common.collect.ClassToInstanceMap; import com.google.common.collect.FluentIterable; +import com.google.common.collect.HashMultimap; import com.google.common.collect.ImmutableMap; +import com.google.common.collect.Lists; import com.google.common.collect.Maps; +import com.google.common.collect.Multimap; import com.google.common.collect.MutableClassToInstanceMap; import com.fasterxml.jackson.annotation.JsonIgnore; @@ -52,6 +57,8 @@ import java.lang.reflect.Proxy; import java.lang.reflect.Type; import java.util.Arrays; +import java.util.Collection; +import java.util.HashMap; import java.util.HashSet; import java.util.Iterator; import java.util.List; @@ -85,16 +92,26 @@ class ProxyInvocationHandler implements InvocationHandler { private final int hashCode = (int) (Math.random() * Integer.MAX_VALUE); private final Set> knownInterfaces; private final ClassToInstanceMap interfaceToProxyCache; - private final Map options; + private final Map options; private final Map jsonOptions; private final Map gettersToPropertyNames; private final Map settersToPropertyNames; ProxyInvocationHandler(Map options) { - this(options, Maps.newHashMap()); + this(bindOptions(options), Maps.newHashMap()); } - private ProxyInvocationHandler(Map options, Map jsonOptions) { + private static Map bindOptions(Map inputOptions) { + HashMap options = Maps.newHashMap(); + for (Map.Entry entry : inputOptions.entrySet()) { + options.put(entry.getKey(), BoundValue.fromExplicitOption(entry.getValue())); + } + + return options; + } + + private ProxyInvocationHandler( + Map options, Map jsonOptions) { this.options = options; this.jsonOptions = jsonOptions; this.knownInterfaces = new HashSet<>(PipelineOptionsFactory.getRegisteredOptions()); @@ -119,21 +136,27 @@ public Object invoke(Object proxy, Method method, Object[] args) { @SuppressWarnings("unchecked") Class clazz = (Class) args[0]; return cloneAs(proxy, clazz); + } else if (args != null && "populateDisplayData".equals(method.getName()) + && args[0] instanceof DisplayData.Builder) { + @SuppressWarnings("unchecked") + DisplayData.Builder builder = (DisplayData.Builder) args[0]; + populateDisplayData(builder); + return Void.TYPE; } String methodName = method.getName(); synchronized (this) { - if (gettersToPropertyNames.keySet().contains(methodName)) { + if (gettersToPropertyNames.containsKey(methodName)) { String propertyName = gettersToPropertyNames.get(methodName); if (!options.containsKey(propertyName)) { // Lazy bind the default to the method. Object value = jsonOptions.containsKey(propertyName) ? getValueFromJson(propertyName, method) : getDefault((PipelineOptions) proxy, method); - options.put(propertyName, value); + options.put(propertyName, BoundValue.fromDefault(value)); } - return options.get(propertyName); + return options.get(propertyName).getValue(); } else if (settersToPropertyNames.containsKey(methodName)) { - options.put(settersToPropertyNames.get(methodName), args[0]); + options.put(settersToPropertyNames.get(methodName), BoundValue.fromExplicitOption(args[0])); return Void.TYPE; } } @@ -141,6 +164,41 @@ public Object invoke(Object proxy, Method method, Object[] args) { + Arrays.toString(args) + "]."); } + /** + * Track whether options values are explicitly set, or retrieved from deserialized JSON/defaults. + */ + private static class BoundValue { + private final Object value; + private final boolean isDefault; + + private BoundValue(Object value, boolean isDefault) { + this.value = value; + this.isDefault = isDefault; + } + + /** + * Create a {@link BoundValue} representing an explicitly set option. + */ + static BoundValue fromExplicitOption(Object value) { + return new BoundValue(value, false); + } + + /** + * Create a {@link BoundValue} representing a default option value. + */ + static BoundValue fromDefault(Object value) { + return new BoundValue(value, true); + } + + Object getValue() { + return value; + } + + boolean isDefault() { + return isDefault; + } + } + /** * Backing implementation for {@link PipelineOptions#as(Class)}. * @@ -209,6 +267,125 @@ public int hashCode() { return hashCode; } + /** + * Populate display data. See {@link HasDisplayData#populateDisplayData}. All explicitly set + * pipeline options will be added as display data. + */ + private void populateDisplayData(DisplayData.Builder builder) { + Set optionSpecs = PipelineOptionsReflector.getOptionSpecs(knownInterfaces); + Multimap optionsMap = buildOptionNameToSpecMap(optionSpecs); + + for (Map.Entry option : options.entrySet()) { + BoundValue boundValue = option.getValue(); + if (boundValue.isDefault()) { + continue; + } + + Object value = boundValue.getValue() == null ? "" : boundValue.getValue(); + DisplayData.Type type = DisplayData.inferType(value); + HashSet specs = new HashSet<>(optionsMap.get(option.getKey())); + + for (PipelineOptionSpec optionSpec : specs) { + Class pipelineInterface = optionSpec.getDefiningInterface(); + if (type != null) { + builder.add(option.getKey(), type, value) + .withNamespace(pipelineInterface); + } else { + builder.add(option.getKey(), value.toString()) + .withNamespace(pipelineInterface); + } + } + } + + for (Map.Entry jsonOption : jsonOptions.entrySet()) { + if (options.containsKey(jsonOption.getKey())) { + // Option overwritten since deserialization; don't re-write + continue; + // TODO: Is it worth removing the JSON option for consistency? + } + HashSet specs = new HashSet<>(optionsMap.get(jsonOption.getKey())); + if (specs.isEmpty()) { + builder.add(jsonOption.getKey(), jsonOption.getValue().toString()) + .withNamespace(UnknownPipelineOptions.class); + } else { + for (PipelineOptionSpec spec : specs) { + Object value = getValueFromJson(jsonOption.getKey(), spec.getGetterMethod()); + DisplayData.Type type = DisplayData.inferType(value); + if (type != null) { + builder.add(jsonOption.getKey(), type, value) + .withNamespace(spec.getDefiningInterface()); + } else { + builder.add(jsonOption.getKey(), value.toString()) + .withNamespace(spec.getDefiningInterface()); + } + } + } + } + } + + /** + * Marker interface use when the original {@link PipelineOptions} interface is not avaialble at + * runtime. This can occur when {@link PipelineOptions} are deserialized from JSON or specified + * on the command line. + * + * To ensure {@link PipelineOptions} type information is available at runtime, register known + * interfaces via {@link PipelineOptionsFactory#register(Class)}. + */ + interface UnknownPipelineOptions extends PipelineOptions {} + + /** + * Construct a mapping from an option name to its {@link PipelineOptions} interface(s) + * declarations. An option may be declared in multiple interfaces. If it is overridden in a + * type hierarchy, only the overriding interface will be included. + */ + private Multimap buildOptionNameToSpecMap( + Set props) { + + Multimap optionsMap = HashMultimap.create(); + for (PipelineOptionSpec prop : props) { + optionsMap.put(prop.getName(), prop); + } + + // Filter out overridden options + for (Map.Entry> entry : optionsMap.asMap().entrySet()) { + + /* Compare all interfaces for an option pairwise (iface1, iface2) to look for type + hierarchies. If one is the base-class of the other, remove it from the output and continue + iterating. + + This is an N^2 operation per-option, but the number of interfaces defining an option + should always be small (usually 1). */ + List specs = Lists.newArrayList(entry.getValue()); + if (specs.size() < 2) { + // Only one known implementing interface, no need to check for inheritence + continue; + } + + for (int i = 0; i < specs.size(); i++) { + Class iface1 = specs.get(i).getDefiningInterface(); + for (int j = i + 1; j < specs.size(); j++) { + Class iface2 = specs.get(j).getDefiningInterface(); + + if (iface1.isAssignableFrom(iface2)) { + optionsMap.remove(entry.getKey(), specs.get(i)); + specs.remove(i); + + // reset iterator indices to increment outer loop. + i--; + j = specs.size(); + } else if (iface2.isAssignableFrom(iface1)) { + optionsMap.remove(entry.getKey(), specs.get(j)); + specs.remove(j); + + j--; + } + } + } + } + + return optionsMap; + } + /** * This will output all the currently set values. This is a relatively costly function * as it will call {@code toString()} on each object that has been set and format @@ -222,7 +399,9 @@ public synchronized String toString() { // Add the options that we received from deserialization sortedOptions.putAll(jsonOptions); // Override with any programmatically set options. - sortedOptions.putAll(options); + for (Map.Entry entry : options.entrySet()) { + sortedOptions.put(entry.getKey(), entry.getValue().getValue()); + } StringBuilder b = new StringBuilder(); b.append("Current Settings:\n"); @@ -347,7 +526,7 @@ public void serialize(PipelineOptions value, JsonGenerator jgen, SerializerProvi // We first filter out any properties that have been modified since // the last serialization of this PipelineOptions and then verify that // they are all serializable. - Map filteredOptions = Maps.newHashMap(handler.options); + Map filteredOptions = Maps.newHashMap(handler.options); removeIgnoredOptions(handler.knownInterfaces, filteredOptions); ensureSerializable(handler.knownInterfaces, filteredOptions); @@ -356,10 +535,22 @@ public void serialize(PipelineOptions value, JsonGenerator jgen, SerializerProvi // instances that have been modified since the previous serialization. Map serializableOptions = Maps.newHashMap(handler.jsonOptions); - serializableOptions.putAll(filteredOptions); + for (Map.Entry entry : filteredOptions.entrySet()) { + serializableOptions.put(entry.getKey(), entry.getValue().getValue()); + } + + jgen.writeStartObject(); jgen.writeFieldName("options"); jgen.writeObject(serializableOptions); + + List> serializedDisplayData = Lists.newArrayList(); + for (DisplayData.Item item : DisplayData.from(value).items()) { + serializedDisplayData.add(MAPPER.convertValue(item, Map.class)); + } + + jgen.writeFieldName("display_data"); + jgen.writeObject(serializedDisplayData); jgen.writeEndObject(); } } @@ -369,7 +560,7 @@ public void serialize(PipelineOptions value, JsonGenerator jgen, SerializerProvi * {@link JsonIgnore @JsonIgnore} from the passed in options using the passed in interfaces. */ private void removeIgnoredOptions( - Set> interfaces, Map options) { + Set> interfaces, Map options) { // Find all the method names that are annotated with JSON ignore. Set jsonIgnoreMethodNames = FluentIterable.from( ReflectHelpers.getClosureOfMethodsOnInterfaces(interfaces)) @@ -394,7 +585,7 @@ public String apply(Method input) { * and deserializable. */ private void ensureSerializable(Set> interfaces, - Map options) throws IOException { + Map options) throws IOException { // Construct a map from property name to the return type of the getter. Map propertyToReturnType = Maps.newHashMap(); for (PropertyDescriptor descriptor @@ -406,16 +597,16 @@ private void ensureSerializable(Set> interfaces } // Attempt to serialize and deserialize each property. - for (Map.Entry entry : options.entrySet()) { + for (Map.Entry entry : options.entrySet()) { try { - String serializedValue = MAPPER.writeValueAsString(entry.getValue()); + String serializedValue = MAPPER.writeValueAsString(entry.getValue().getValue()); JavaType type = MAPPER.getTypeFactory() .constructType(propertyToReturnType.get(entry.getKey())); MAPPER.readValue(serializedValue, type); } catch (Exception e) { throw new IOException(String.format( "Failed to serialize and deserialize property '%s' with value '%s'", - entry.getKey(), entry.getValue()), e); + entry.getKey(), entry.getValue().getValue()), e); } } } @@ -435,7 +626,7 @@ public PipelineOptions deserialize(JsonParser jp, DeserializationContext ctxt) fields.put(field.getKey(), field.getValue()); } PipelineOptions options = - new ProxyInvocationHandler(Maps.newHashMap(), fields) + new ProxyInvocationHandler(Maps.newHashMap(), fields) .as(PipelineOptions.class); return options; } diff --git a/sdks/java/core/src/main/java/org/apache/beam/sdk/transforms/display/DisplayData.java b/sdks/java/core/src/main/java/org/apache/beam/sdk/transforms/display/DisplayData.java index 6065dc486180..a1037a8db0d1 100644 --- a/sdks/java/core/src/main/java/org/apache/beam/sdk/transforms/display/DisplayData.java +++ b/sdks/java/core/src/main/java/org/apache/beam/sdk/transforms/display/DisplayData.java @@ -111,6 +111,21 @@ public Map asMap() { return entries; } + @Override + public int hashCode() { + return entries.hashCode(); + } + + @Override + public boolean equals(Object obj) { + if (obj instanceof DisplayData) { + DisplayData that = (DisplayData) obj; + return Objects.equals(this.entries, that.entries); + } + + return false; + } + @Override public String toString() { StringBuilder builder = new StringBuilder(); @@ -844,7 +859,6 @@ public ItemBuilder addIfNotDefault(String key, boolean value, boolean defaultVal @Override public ItemBuilder add(String key, Instant value) { - checkNotNull(value); return addItemIf(true, key, Type.TIMESTAMP, value); } @@ -861,7 +875,6 @@ public ItemBuilder addIfNotDefault( @Override public ItemBuilder add(String key, Duration value) { - checkNotNull(value); return addItemIf(true, key, Type.DURATION, value); } @@ -878,7 +891,6 @@ public ItemBuilder addIfNotDefault( @Override public ItemBuilder add(String key, Class value) { - checkNotNull(value); return addItemIf(true, key, Type.JAVA_CLASS, value); } @@ -912,17 +924,17 @@ public ItemBuilder addIfNotDefault( @Override public ItemBuilder add(String key, Type type, Object value) { - checkNotNull(value); checkNotNull(type); return addItemIf(true, key, type, value); } private ItemBuilder addItemIf(boolean condition, String key, Type type, Object value) { - checkNotNull(key); - checkArgument(!key.isEmpty()); - + checkNotNull(key, "Display data keys cannot be null or empty."); + checkArgument(!key.isEmpty(), "Display data keys cannot be null or empty."); commitLatest(); + if (condition) { + checkNotNull(value, "Display data values cannot be null. Key: [%s]", key); latestItem = Item.create(latestNs, key, type, value); } diff --git a/sdks/java/core/src/test/java/org/apache/beam/sdk/options/PipelineOptionsReflectorTest.java b/sdks/java/core/src/test/java/org/apache/beam/sdk/options/PipelineOptionsReflectorTest.java new file mode 100644 index 000000000000..0de9c9cf2b21 --- /dev/null +++ b/sdks/java/core/src/test/java/org/apache/beam/sdk/options/PipelineOptionsReflectorTest.java @@ -0,0 +1,197 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you 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.beam.sdk.options; + +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.allOf; +import static org.hamcrest.Matchers.hasItem; +import static org.hamcrest.Matchers.is; +import static org.hamcrest.Matchers.isOneOf; +import static org.hamcrest.Matchers.not; + +import com.google.common.collect.ImmutableSet; +import org.hamcrest.FeatureMatcher; +import org.hamcrest.Matcher; +import org.hamcrest.Matchers; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +import java.util.Set; + +/** + * Unit tests for {@link PipelineOptionsReflector}. + */ +@RunWith(JUnit4.class) +public class PipelineOptionsReflectorTest { + @Test + public void testGetOptionSpecs() throws NoSuchMethodException { + Set properties = + PipelineOptionsReflector.getOptionSpecs(SimpleOptions.class); + + assertThat(properties, Matchers.hasItems(PipelineOptionSpec.of( + SimpleOptions.class, "foo", SimpleOptions.class.getDeclaredMethod("getFoo")))); + } + + interface SimpleOptions extends PipelineOptions { + String getFoo(); + void setFoo(String value); + } + + @Test + public void testFiltersNonGetterMethods() { + Set properties = + PipelineOptionsReflector.getOptionSpecs(OnlyTwoValidGetters.class); + + assertThat(properties, not(hasItem(hasName(isOneOf("misspelled", "hasParameter", "prefix"))))); + } + + interface OnlyTwoValidGetters extends PipelineOptions { + String getFoo(); + void setFoo(String value); + + boolean isBar(); + void setBar(boolean value); + + String gtMisspelled(); + void setMisspelled(String value); + + String getHasParameter(String value); + void setHasParameter(String value); + + String noPrefix(); + void setNoPrefix(String value); + } + + @Test + public void testBaseClassOptions() { + Set props = + PipelineOptionsReflector.getOptionSpecs(ExtendsSimpleOptions.class); + + assertThat(props, Matchers.hasItem( + allOf(hasName("foo"), hasClass(SimpleOptions.class)))); + assertThat(props, Matchers.hasItem( + allOf(hasName("foo"), hasClass(ExtendsSimpleOptions.class)))); + assertThat(props, Matchers.hasItem( + allOf(hasName("bar"), hasClass(ExtendsSimpleOptions.class)))); + } + + interface ExtendsSimpleOptions extends SimpleOptions { + @Override String getFoo(); + @Override void setFoo(String value); + + String getBar(); + void setBar(String value); + } + + @Test + public void testExcludesNonPipelineOptionsMethods() { + Set properties = + PipelineOptionsReflector.getOptionSpecs(ExtendsNonPipelineOptions.class); + + assertThat(properties, not(hasItem(hasName("foo")))); + } + + interface NoExtendsClause { + String getFoo(); + void setFoo(String value); + } + + interface ExtendsNonPipelineOptions extends NoExtendsClause, PipelineOptions {} + + @Test + public void testExcludesHiddenInterfaces() { + Set properties = + PipelineOptionsReflector.getOptionSpecs(Hidden.class); + + assertThat(properties, not(hasItem(hasName("foo")))); + } + + @org.apache.beam.sdk.options.Hidden + interface Hidden extends PipelineOptions { + String getFoo(); + void setFoo(String value); + } + + @Test + public void testMultipleInputInterfaces() { + Set> interfaces = ImmutableSet + .>builder() + .add(BaseOptions.class) + .add(ExtendOptions1.class) + .add(ExtendOptions2.class) + .build(); + + Set props = PipelineOptionsReflector.getOptionSpecs(interfaces); + + assertThat(props, Matchers.hasItem(allOf(hasName("baseOption"), hasClass(BaseOptions.class)))); + assertThat(props, Matchers.hasItem( + allOf(hasName("extendOption1"), hasClass(ExtendOptions1.class)))); + assertThat(props, Matchers.hasItem( + allOf(hasName("extendOption2"), hasClass(ExtendOptions2.class)))); + } + + interface BaseOptions extends PipelineOptions { + String getBaseOption(); + void setBaseOption(String value); + } + + interface ExtendOptions1 extends BaseOptions { + String getExtendOption1(); + void setExtendOption1(String value); + } + + interface ExtendOptions2 extends BaseOptions { + String getExtendOption2(); + void setExtendOption2(String value); + } + + private static Matcher hasName(String name) { + return hasName(is(name)); + } + + private static Matcher hasName(Matcher matcher) { + return new FeatureMatcher(matcher, "name", "name") { + @Override + protected String featureValueOf(PipelineOptionSpec actual) { + return actual.getName(); + } + }; + } + + private static Matcher hasClass(Class clazz) { + return new FeatureMatcher>( + Matchers.>is(clazz), "defining class", "class") { + @Override + protected Class featureValueOf(PipelineOptionSpec actual) { + return actual.getDefiningInterface(); + } + }; + } + + private static Matcher hasGetter(String methodName) { + return new FeatureMatcher( + is(methodName), "getter method", "name") { + @Override + protected String featureValueOf(PipelineOptionSpec actual) { + return actual.getGetterMethod().getName(); + } + }; + } +} diff --git a/sdks/java/core/src/test/java/org/apache/beam/sdk/options/ProxyInvocationHandlerTest.java b/sdks/java/core/src/test/java/org/apache/beam/sdk/options/ProxyInvocationHandlerTest.java index 7f0fa142d353..56cec1758918 100644 --- a/sdks/java/core/src/test/java/org/apache/beam/sdk/options/ProxyInvocationHandlerTest.java +++ b/sdks/java/core/src/test/java/org/apache/beam/sdk/options/ProxyInvocationHandlerTest.java @@ -17,12 +17,23 @@ */ package org.apache.beam.sdk.options; +import static org.apache.beam.sdk.transforms.display.DisplayDataMatchers.hasDisplayItem; +import static org.apache.beam.sdk.transforms.display.DisplayDataMatchers.hasKey; +import static org.apache.beam.sdk.transforms.display.DisplayDataMatchers.hasNamespace; +import static org.apache.beam.sdk.transforms.display.DisplayDataMatchers.hasType; +import static org.apache.beam.sdk.transforms.display.DisplayDataMatchers.hasValue; +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.allOf; +import static org.hamcrest.Matchers.hasItem; +import static org.hamcrest.Matchers.not; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertNull; import static org.junit.Assert.assertTrue; +import org.apache.beam.sdk.transforms.display.DisplayData; + import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; @@ -33,12 +44,19 @@ import com.fasterxml.jackson.databind.JsonMappingException; import com.fasterxml.jackson.databind.ObjectMapper; +import org.hamcrest.Matchers; +import org.joda.time.Instant; import org.junit.Rule; import org.junit.Test; import org.junit.rules.ExpectedException; +import org.junit.rules.ExternalResource; +import org.junit.rules.TestRule; import org.junit.runner.RunWith; import org.junit.runners.JUnit4; +import java.io.IOException; +import java.io.Serializable; +import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.Objects; @@ -48,6 +66,14 @@ @RunWith(JUnit4.class) public class ProxyInvocationHandlerTest { @Rule public ExpectedException expectedException = ExpectedException.none(); + @Rule public TestRule resetPipelineOptionsRegistry = new ExternalResource() { + @Override + protected void before() { + PipelineOptionsFactory.resetRegistry(); + } + }; + + private static final ObjectMapper MAPPER = new ObjectMapper(); /** A test interface with some primitives and objects. */ public static interface Simple extends PipelineOptions { @@ -432,6 +458,19 @@ public void testPartialMethodConflictProvidesSameValue() throws Exception { assertEquals(5, partialMethodConflict.as(Simple.class).getPrimitive()); } + @Test + public void testResetRegistry() { + Set> defaultRegistry = + new HashSet<>(PipelineOptionsFactory.getRegisteredOptions()); + assertThat(defaultRegistry, not(hasItem(FooOptions.class))); + + PipelineOptionsFactory.register(FooOptions.class); + assertThat(PipelineOptionsFactory.getRegisteredOptions(), hasItem(FooOptions.class)); + + PipelineOptionsFactory.resetRegistry(); + assertEquals(defaultRegistry, PipelineOptionsFactory.getRegisteredOptions()); + } + @Test public void testJsonConversionForDefault() throws Exception { PipelineOptions options = PipelineOptionsFactory.create(); @@ -683,10 +722,233 @@ public void testJsonConversionOfSerializableWithMetadataProperty() throws Except assertEquals("TestString", options2.getValue().getValue()); } + @Test + public void testDisplayDataItemProperties() { + PipelineOptions options = PipelineOptionsFactory.create(); + options.setTempLocation("myTemp"); + DisplayData displayData = DisplayData.from(options); + + assertThat(displayData, hasDisplayItem(allOf( + hasKey("tempLocation"), + hasType(DisplayData.Type.STRING), + hasValue("myTemp"), + hasNamespace(PipelineOptions.class) + ))); + } + + @Test + public void testDisplayDataTypes() { + Instant now = Instant.now(); + + TypedOptions options = PipelineOptionsFactory.as(TypedOptions.class); + options.setInteger(1234); + options.setTimestamp(now); + options.setJavaClass(ProxyInvocationHandlerTest.class); + options.setObject(new Serializable() { + @Override + public String toString() { + return "foobar"; + } + }); + + DisplayData displayData = DisplayData.from(options); + + assertThat(displayData, hasDisplayItem("integer", 1234)); + assertThat(displayData, hasDisplayItem("timestamp", now)); + assertThat(displayData, hasDisplayItem("javaClass", ProxyInvocationHandlerTest.class)); + assertThat(displayData, hasDisplayItem("object", "foobar")); + } + + interface TypedOptions extends PipelineOptions { + int getInteger(); + void setInteger(int value); + + Instant getTimestamp(); + void setTimestamp(Instant value); + + Class getJavaClass(); + void setJavaClass(Class value); + + Object getObject(); + void setObject(Object value); + } + + @Test + public void testDisplayDataInheritanceNamespace() { + ExtendsBaseOptions options = PipelineOptionsFactory.as(ExtendsBaseOptions.class); + options.setFoo("bar"); + + DisplayData displayData = DisplayData.from(options); + + assertThat(displayData, hasDisplayItem(allOf( + hasKey("foo"), + hasValue("bar"), + hasNamespace(ExtendsBaseOptions.class) + ))); + } + + interface BaseOptions extends PipelineOptions { + String getFoo(); + void setFoo(String value); + } + + interface ExtendsBaseOptions extends BaseOptions { + @Override String getFoo(); + @Override void setFoo(String value); + } + + @Test + public void testDisplayDataExcludedFromOverriddenBaseClass() { + ExtendsBaseOptions options = PipelineOptionsFactory.as(ExtendsBaseOptions.class); + options.setFoo("bar"); + + DisplayData displayData = DisplayData.from(options); + assertThat(displayData, not(hasDisplayItem(hasNamespace(BaseOptions.class)))); + } + + @Test + public void testDisplayDataIncludedForDisjointInterfaceHierarchies() { + FooOptions fooOptions = PipelineOptionsFactory.as(FooOptions.class); + fooOptions.setFoo("foo"); + + BarOptions barOptions = fooOptions.as(BarOptions.class); + barOptions.setBar("bar"); + + DisplayData data = DisplayData.from(barOptions); + assertThat(data, hasDisplayItem(allOf(hasKey("foo"), hasNamespace(FooOptions.class)))); + assertThat(data, hasDisplayItem(allOf(hasKey("bar"), hasNamespace(BarOptions.class)))); + } + + interface FooOptions extends PipelineOptions { + String getFoo(); + void setFoo(String value); + } + + interface BarOptions extends PipelineOptions { + String getBar(); + void setBar(String value); + } + + @Test + public void testDisplayDataExcludesDefaultValues() { + PipelineOptions options = PipelineOptionsFactory.as(HasDefaults.class); + DisplayData data = DisplayData.from(options); + + assertThat(data, not(hasDisplayItem(hasKey("foo")))); + } + + interface HasDefaults extends PipelineOptions { + @Default.String("bar") + String getFoo(); + void setFoo(String value); + } + + @Test + public void testDisplayDataExcludesValuesAccessedButNeverSet() { + HasDefaults options = PipelineOptionsFactory.as(HasDefaults.class); + assertEquals("bar", options.getFoo()); + + DisplayData data = DisplayData.from(options); + assertThat(data, not(hasDisplayItem(hasKey("foo")))); + } + + @Test + public void testDisplayDataIncludesExplicitlySetDefaults() { + HasDefaults options = PipelineOptionsFactory.as(HasDefaults.class); + options.setFoo("bar"); + + DisplayData data = DisplayData.from(options); + assertThat(data, hasDisplayItem(hasKey("foo"))); + } + + @Test + public void testDisplayDataNullValuesConvertedToEmptyString() { + FooOptions options = PipelineOptionsFactory.as(FooOptions.class); + options.setFoo(null); + + DisplayData data = DisplayData.from(options); + assertThat(data, hasDisplayItem("foo", "")); + } + + @Test + public void testDisplayDataJsonSerialization() throws IOException { + FooOptions options = PipelineOptionsFactory.as(FooOptions.class); + options.setFoo("bar"); + + @SuppressWarnings("unchecked") + Map map = MAPPER.readValue(MAPPER.writeValueAsBytes(options), Map.class); + + assertThat("main pipeline options data keyed as 'options'", map, Matchers.hasKey("options")); + assertThat("display data keyed as 'display_data'", map, Matchers.hasKey("display_data")); + + Map expectedDisplayItem = ImmutableMap.builder() + .put("namespace", FooOptions.class.getName()) + .put("key", "foo") + .put("value", "bar") + .put("type", "STRING") + .build(); + + @SuppressWarnings("unchecked") + List> deserializedDisplayData = (List>) map.get("display_data"); + assertThat(deserializedDisplayData, hasItem(expectedDisplayItem)); + } + + @Test + public void testDisplayDataFromDeserializedJson() throws Exception { +// TODO PipelineOptionsFactory.register(FooOptions.class); + FooOptions options = PipelineOptionsFactory.as(FooOptions.class); + options.setFoo("bar"); + DisplayData data = DisplayData.from(options); + assertThat(data, hasDisplayItem("foo", "bar")); + + FooOptions deserializedOptions = serializeDeserialize(FooOptions.class, options); + DisplayData dataAfterDeserialization = DisplayData.from(deserializedOptions); + assertEquals(data, dataAfterDeserialization); + } + + @Test + public void testDisplayDataDeserializationWithRegistration() throws Exception { + PipelineOptionsFactory.register(HasClassOptions.class); + HasClassOptions options = PipelineOptionsFactory.as(HasClassOptions.class); + options.setClassOption(ProxyInvocationHandlerTest.class); + + PipelineOptions deserializedOptions = serializeDeserialize(PipelineOptions.class, options); + DisplayData displayData = DisplayData.from(deserializedOptions); + assertThat(displayData, hasDisplayItem("classOption", ProxyInvocationHandlerTest.class)); + } + + @Test + public void testDisplayDataMissingPipelineOptionsRegistration() throws Exception { + HasClassOptions options = PipelineOptionsFactory.as(HasClassOptions.class); + options.setClassOption(ProxyInvocationHandlerTest.class); + + PipelineOptions deserializedOptions = serializeDeserialize(PipelineOptions.class, options); + DisplayData displayData = DisplayData.from(deserializedOptions); + String expectedJsonValue = MAPPER.writeValueAsString(ProxyInvocationHandlerTest.class); + assertThat(displayData, hasDisplayItem("classOption", expectedJsonValue)); + } + + interface HasClassOptions extends PipelineOptions { + Class getClassOption(); + void setClassOption(Class value); + } + + @Test + public void testDisplayDataJsonValueSetAfterDeserialization() throws Exception { + FooOptions options = PipelineOptionsFactory.as(FooOptions.class); + options.setFoo("bar"); + DisplayData data = DisplayData.from(options); + assertThat(data, hasDisplayItem("foo", "bar")); + + FooOptions deserializedOptions = serializeDeserialize(FooOptions.class, options); + deserializedOptions.setFoo("baz"); + DisplayData dataAfterDeserialization = DisplayData.from(deserializedOptions); + assertThat(dataAfterDeserialization, hasDisplayItem("foo", "baz")); + } + private T serializeDeserialize(Class kls, PipelineOptions options) throws Exception { - ObjectMapper mapper = new ObjectMapper(); - String value = mapper.writeValueAsString(options); - return mapper.readValue(value, PipelineOptions.class).as(kls); + String value = MAPPER.writeValueAsString(options); + return MAPPER.readValue(value, PipelineOptions.class).as(kls); } } diff --git a/sdks/java/core/src/test/java/org/apache/beam/sdk/transforms/display/DisplayDataTest.java b/sdks/java/core/src/test/java/org/apache/beam/sdk/transforms/display/DisplayDataTest.java index 106c44139b5e..05d0f6f0a046 100644 --- a/sdks/java/core/src/test/java/org/apache/beam/sdk/transforms/display/DisplayDataTest.java +++ b/sdks/java/core/src/test/java/org/apache/beam/sdk/transforms/display/DisplayDataTest.java @@ -383,6 +383,31 @@ public void populateDisplayData(Builder builder) { .testEquals(); } + @Test + public void testDisplayDataEquality() { + HasDisplayData component1 = new HasDisplayData() { + @Override + public void populateDisplayData(Builder builder) { + builder.add("foo", "bar"); + } + }; + HasDisplayData component2 = new HasDisplayData() { + @Override + public void populateDisplayData(Builder builder) { + builder.add("foo", "bar"); + } + }; + + DisplayData component1DisplayData1 = DisplayData.from(component1); + DisplayData component1DisplayData2 = DisplayData.from(component1); + DisplayData component2DisplayData = DisplayData.from(component2); + + new EqualsTester() + .addEqualityGroup(component1DisplayData1, component1DisplayData2) + .addEqualityGroup(component2DisplayData) + .testEquals(); + } + @Test public void testAnonymousClassNamespace() { DisplayData data = From fa4474897aefc7a8687deb97e5b1d50169a6ce61 Mon Sep 17 00:00:00 2001 From: Scott Wegner Date: Wed, 20 Apr 2016 09:18:36 -0700 Subject: [PATCH 02/13] fixup! spelling error --- .../org/apache/beam/sdk/options/ProxyInvocationHandler.java | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/sdks/java/core/src/main/java/org/apache/beam/sdk/options/ProxyInvocationHandler.java b/sdks/java/core/src/main/java/org/apache/beam/sdk/options/ProxyInvocationHandler.java index 139a31a99bff..1cbb06a80a50 100644 --- a/sdks/java/core/src/main/java/org/apache/beam/sdk/options/ProxyInvocationHandler.java +++ b/sdks/java/core/src/main/java/org/apache/beam/sdk/options/ProxyInvocationHandler.java @@ -324,7 +324,7 @@ private void populateDisplayData(DisplayData.Builder builder) { } /** - * Marker interface use when the original {@link PipelineOptions} interface is not avaialble at + * Marker interface use when the original {@link PipelineOptions} interface is not available at * runtime. This can occur when {@link PipelineOptions} are deserialized from JSON or specified * on the command line. * @@ -546,7 +546,9 @@ public void serialize(PipelineOptions value, JsonGenerator jgen, SerializerProvi List> serializedDisplayData = Lists.newArrayList(); for (DisplayData.Item item : DisplayData.from(value).items()) { - serializedDisplayData.add(MAPPER.convertValue(item, Map.class)); + @SuppressWarnings("unchecked") + Map serializedItem = MAPPER.convertValue(item, Map.class); + serializedDisplayData.add(serializedItem); } jgen.writeFieldName("display_data"); From bbcce82d0a6f8f8cbaec83610981effbc0e2f439 Mon Sep 17 00:00:00 2001 From: Scott Wegner Date: Wed, 20 Apr 2016 09:26:18 -0700 Subject: [PATCH 03/13] fixup! line spacing --- .../sdk/options/ProxyInvocationHandler.java | 28 ++++++++++--------- .../options/PipelineOptionsReflectorTest.java | 4 +-- 2 files changed, 17 insertions(+), 15 deletions(-) diff --git a/sdks/java/core/src/main/java/org/apache/beam/sdk/options/ProxyInvocationHandler.java b/sdks/java/core/src/main/java/org/apache/beam/sdk/options/ProxyInvocationHandler.java index 1cbb06a80a50..16bd40326093 100644 --- a/sdks/java/core/src/main/java/org/apache/beam/sdk/options/ProxyInvocationHandler.java +++ b/sdks/java/core/src/main/java/org/apache/beam/sdk/options/ProxyInvocationHandler.java @@ -301,8 +301,8 @@ private void populateDisplayData(DisplayData.Builder builder) { if (options.containsKey(jsonOption.getKey())) { // Option overwritten since deserialization; don't re-write continue; - // TODO: Is it worth removing the JSON option for consistency? } + HashSet specs = new HashSet<>(optionsMap.get(jsonOption.getKey())); if (specs.isEmpty()) { builder.add(jsonOption.getKey(), jsonOption.getValue().toString()) @@ -324,7 +324,7 @@ private void populateDisplayData(DisplayData.Builder builder) { } /** - * Marker interface use when the original {@link PipelineOptions} interface is not available at + * Marker interface used when the original {@link PipelineOptions} interface is not available at * runtime. This can occur when {@link PipelineOptions} are deserialized from JSON or specified * on the command line. * @@ -357,7 +357,7 @@ private Multimap buildOptionNameToSpecMap( should always be small (usually 1). */ List specs = Lists.newArrayList(entry.getValue()); if (specs.size() < 2) { - // Only one known implementing interface, no need to check for inheritence + // Only one known implementing interface, no need to check for inheritance continue; } @@ -370,13 +370,16 @@ should always be small (usually 1). */ optionsMap.remove(entry.getKey(), specs.get(i)); specs.remove(i); - // reset iterator indices to increment outer loop. + // Removed element at current "i" index. Set iterators to re-evaluate + // new "i" element in outer loop. i--; j = specs.size(); } else if (iface2.isAssignableFrom(iface1)) { optionsMap.remove(entry.getKey(), specs.get(j)); specs.remove(j); + // Removed element at current "j" index. Set iterator to re-evaluate + // new "j" element in inner-loop. j--; } } @@ -539,20 +542,19 @@ public void serialize(PipelineOptions value, JsonGenerator jgen, SerializerProvi serializableOptions.put(entry.getKey(), entry.getValue().getValue()); } - jgen.writeStartObject(); jgen.writeFieldName("options"); jgen.writeObject(serializableOptions); - List> serializedDisplayData = Lists.newArrayList(); - for (DisplayData.Item item : DisplayData.from(value).items()) { - @SuppressWarnings("unchecked") - Map serializedItem = MAPPER.convertValue(item, Map.class); - serializedDisplayData.add(serializedItem); - } + List> serializedDisplayData = Lists.newArrayList(); + for (DisplayData.Item item : DisplayData.from(value).items()) { + @SuppressWarnings("unchecked") + Map serializedItem = MAPPER.convertValue(item, Map.class); + serializedDisplayData.add(serializedItem); + } - jgen.writeFieldName("display_data"); - jgen.writeObject(serializedDisplayData); + jgen.writeFieldName("display_data"); + jgen.writeObject(serializedDisplayData); jgen.writeEndObject(); } } diff --git a/sdks/java/core/src/test/java/org/apache/beam/sdk/options/PipelineOptionsReflectorTest.java b/sdks/java/core/src/test/java/org/apache/beam/sdk/options/PipelineOptionsReflectorTest.java index 0de9c9cf2b21..b22f478a176f 100644 --- a/sdks/java/core/src/test/java/org/apache/beam/sdk/options/PipelineOptionsReflectorTest.java +++ b/sdks/java/core/src/test/java/org/apache/beam/sdk/options/PipelineOptionsReflectorTest.java @@ -88,8 +88,8 @@ public void testBaseClassOptions() { allOf(hasName("foo"), hasClass(SimpleOptions.class)))); assertThat(props, Matchers.hasItem( allOf(hasName("foo"), hasClass(ExtendsSimpleOptions.class)))); - assertThat(props, Matchers.hasItem( - allOf(hasName("bar"), hasClass(ExtendsSimpleOptions.class)))); + assertThat(props, Matchers.hasItem( + allOf(hasName("bar"), hasClass(ExtendsSimpleOptions.class)))); } interface ExtendsSimpleOptions extends SimpleOptions { From 24bd9b7e0ee4bbb205c2cdbf5c68fcb14eeae180 Mon Sep 17 00:00:00 2001 From: Scott Wegner Date: Wed, 20 Apr 2016 09:29:23 -0700 Subject: [PATCH 04/13] fixup! Use ImmutableSet.of shorthand method --- .../sdk/options/PipelineOptionsReflectorTest.java | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/sdks/java/core/src/test/java/org/apache/beam/sdk/options/PipelineOptionsReflectorTest.java b/sdks/java/core/src/test/java/org/apache/beam/sdk/options/PipelineOptionsReflectorTest.java index b22f478a176f..bb2fedf645ba 100644 --- a/sdks/java/core/src/test/java/org/apache/beam/sdk/options/PipelineOptionsReflectorTest.java +++ b/sdks/java/core/src/test/java/org/apache/beam/sdk/options/PipelineOptionsReflectorTest.java @@ -131,20 +131,19 @@ interface Hidden extends PipelineOptions { @Test public void testMultipleInputInterfaces() { - Set> interfaces = ImmutableSet - .>builder() - .add(BaseOptions.class) - .add(ExtendOptions1.class) - .add(ExtendOptions2.class) - .build(); + Set> interfaces = + ImmutableSet.>of( + BaseOptions.class, + ExtendOptions1.class, + ExtendOptions2.class); Set props = PipelineOptionsReflector.getOptionSpecs(interfaces); assertThat(props, Matchers.hasItem(allOf(hasName("baseOption"), hasClass(BaseOptions.class)))); assertThat(props, Matchers.hasItem( allOf(hasName("extendOption1"), hasClass(ExtendOptions1.class)))); - assertThat(props, Matchers.hasItem( - allOf(hasName("extendOption2"), hasClass(ExtendOptions2.class)))); + assertThat(props, Matchers.hasItem( + allOf(hasName("extendOption2"), hasClass(ExtendOptions2.class)))); } interface BaseOptions extends PipelineOptions { From 9c9c9af9df142c0f9754c0947075bfc3cbea19e4 Mon Sep 17 00:00:00 2001 From: Scott Wegner Date: Wed, 20 Apr 2016 09:35:03 -0700 Subject: [PATCH 05/13] fixup! improve test for default value being set --- .../apache/beam/sdk/options/ProxyInvocationHandlerTest.java | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/sdks/java/core/src/test/java/org/apache/beam/sdk/options/ProxyInvocationHandlerTest.java b/sdks/java/core/src/test/java/org/apache/beam/sdk/options/ProxyInvocationHandlerTest.java index 56cec1758918..aa6273ba4889 100644 --- a/sdks/java/core/src/test/java/org/apache/beam/sdk/options/ProxyInvocationHandlerTest.java +++ b/sdks/java/core/src/test/java/org/apache/beam/sdk/options/ProxyInvocationHandlerTest.java @@ -855,10 +855,11 @@ public void testDisplayDataExcludesValuesAccessedButNeverSet() { @Test public void testDisplayDataIncludesExplicitlySetDefaults() { HasDefaults options = PipelineOptionsFactory.as(HasDefaults.class); - options.setFoo("bar"); + String defaultValue = options.getFoo(); + options.setFoo(defaultValue); DisplayData data = DisplayData.from(options); - assertThat(data, hasDisplayItem(hasKey("foo"))); + assertThat(data, hasDisplayItem("foo", defaultValue)); } @Test From 41f171feb66c62f6fa7878579b98113985bf2994 Mon Sep 17 00:00:00 2001 From: Scott Wegner Date: Wed, 20 Apr 2016 09:38:47 -0700 Subject: [PATCH 06/13] fixup! remove TODO --- .../org/apache/beam/sdk/options/ProxyInvocationHandlerTest.java | 1 - 1 file changed, 1 deletion(-) diff --git a/sdks/java/core/src/test/java/org/apache/beam/sdk/options/ProxyInvocationHandlerTest.java b/sdks/java/core/src/test/java/org/apache/beam/sdk/options/ProxyInvocationHandlerTest.java index aa6273ba4889..b53000dba353 100644 --- a/sdks/java/core/src/test/java/org/apache/beam/sdk/options/ProxyInvocationHandlerTest.java +++ b/sdks/java/core/src/test/java/org/apache/beam/sdk/options/ProxyInvocationHandlerTest.java @@ -896,7 +896,6 @@ public void testDisplayDataJsonSerialization() throws IOException { @Test public void testDisplayDataFromDeserializedJson() throws Exception { -// TODO PipelineOptionsFactory.register(FooOptions.class); FooOptions options = PipelineOptionsFactory.as(FooOptions.class); options.setFoo("bar"); DisplayData data = DisplayData.from(options); From 814159e0e6cf1b3b17928baf214cd9f6f772a342 Mon Sep 17 00:00:00 2001 From: Scott Wegner Date: Wed, 20 Apr 2016 10:01:48 -0700 Subject: [PATCH 07/13] fixup! Improve javadoc and loop variable --- .../apache/beam/sdk/options/ProxyInvocationHandler.java | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/sdks/java/core/src/main/java/org/apache/beam/sdk/options/ProxyInvocationHandler.java b/sdks/java/core/src/main/java/org/apache/beam/sdk/options/ProxyInvocationHandler.java index 16bd40326093..dea39f32013d 100644 --- a/sdks/java/core/src/main/java/org/apache/beam/sdk/options/ProxyInvocationHandler.java +++ b/sdks/java/core/src/main/java/org/apache/beam/sdk/options/ProxyInvocationHandler.java @@ -324,12 +324,13 @@ private void populateDisplayData(DisplayData.Builder builder) { } /** - * Marker interface used when the original {@link PipelineOptions} interface is not available at + * Marker interface used when the original {@link PipelineOptions} interface is not known at * runtime. This can occur when {@link PipelineOptions} are deserialized from JSON or specified * on the command line. * - * To ensure {@link PipelineOptions} type information is available at runtime, register known - * interfaces via {@link PipelineOptionsFactory#register(Class)}. + *

    Pipeline authors can ensure {@link PipelineOptions} type information is available at + * runtime by explicitly registering {@link PipelineOptions options} interfaces via + * {@link PipelineOptionsFactory#register(Class)}. */ interface UnknownPipelineOptions extends PipelineOptions {} @@ -361,7 +362,7 @@ should always be small (usually 1). */ continue; } - for (int i = 0; i < specs.size(); i++) { + for (int i = 0; i < specs.size() - 1; i++) { Class iface1 = specs.get(i).getDefiningInterface(); for (int j = i + 1; j < specs.size(); j++) { Class iface2 = specs.get(j).getDefiningInterface(); From afdf1146816a203cbfbebf8f5187cdd8ddc44d9e Mon Sep 17 00:00:00 2001 From: Scott Wegner Date: Thu, 21 Apr 2016 14:08:33 -0700 Subject: [PATCH 08/13] fixup! improve javadoc --- .../org/apache/beam/sdk/options/ProxyInvocationHandler.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/sdks/java/core/src/main/java/org/apache/beam/sdk/options/ProxyInvocationHandler.java b/sdks/java/core/src/main/java/org/apache/beam/sdk/options/ProxyInvocationHandler.java index dea39f32013d..fdf383a0f0af 100644 --- a/sdks/java/core/src/main/java/org/apache/beam/sdk/options/ProxyInvocationHandler.java +++ b/sdks/java/core/src/main/java/org/apache/beam/sdk/options/ProxyInvocationHandler.java @@ -66,7 +66,6 @@ import java.util.Set; import java.util.SortedMap; import java.util.TreeMap; - import javax.annotation.concurrent.ThreadSafe; /** @@ -165,7 +164,7 @@ public Object invoke(Object proxy, Method method, Object[] args) { } /** - * Track whether options values are explicitly set, or retrieved from deserialized JSON/defaults. + * Track whether options values are explicitly set, or retrieved from defaults. */ private static class BoundValue { private final Object value; From 6acbee004e9ee04a50c7aa39e7e5be9cc53703c7 Mon Sep 17 00:00:00 2001 From: Scott Wegner Date: Thu, 21 Apr 2016 14:21:45 -0700 Subject: [PATCH 09/13] fixup! Use AutoValue for ProxyInvocationHandler.BoundValue --- .../sdk/options/ProxyInvocationHandler.java | 25 +++++++------------ 1 file changed, 9 insertions(+), 16 deletions(-) diff --git a/sdks/java/core/src/main/java/org/apache/beam/sdk/options/ProxyInvocationHandler.java b/sdks/java/core/src/main/java/org/apache/beam/sdk/options/ProxyInvocationHandler.java index fdf383a0f0af..f0b3f083d271 100644 --- a/sdks/java/core/src/main/java/org/apache/beam/sdk/options/ProxyInvocationHandler.java +++ b/sdks/java/core/src/main/java/org/apache/beam/sdk/options/ProxyInvocationHandler.java @@ -24,6 +24,7 @@ import org.apache.beam.sdk.util.InstanceBuilder; import org.apache.beam.sdk.util.common.ReflectHelpers; +import com.google.auto.value.AutoValue; import com.google.common.base.Defaults; import com.google.common.base.Function; import com.google.common.base.Preconditions; @@ -166,35 +167,27 @@ public Object invoke(Object proxy, Method method, Object[] args) { /** * Track whether options values are explicitly set, or retrieved from defaults. */ - private static class BoundValue { - private final Object value; - private final boolean isDefault; + @AutoValue + abstract static class BoundValue { + abstract Object getValue(); + abstract boolean isDefault(); - private BoundValue(Object value, boolean isDefault) { - this.value = value; - this.isDefault = isDefault; + private static BoundValue of(Object value, boolean isDefault) { + return new AutoValue_ProxyInvocationHandler_BoundValue(value, isDefault); } /** * Create a {@link BoundValue} representing an explicitly set option. */ static BoundValue fromExplicitOption(Object value) { - return new BoundValue(value, false); + return BoundValue.of(value, false); } /** * Create a {@link BoundValue} representing a default option value. */ static BoundValue fromDefault(Object value) { - return new BoundValue(value, true); - } - - Object getValue() { - return value; - } - - boolean isDefault() { - return isDefault; + return BoundValue.of(value, true); } } From 3e7612003b4dbe01b4dc30b7bbea355ff9db02c8 Mon Sep 17 00:00:00 2001 From: Scott Wegner Date: Thu, 21 Apr 2016 14:26:07 -0700 Subject: [PATCH 10/13] fixup! Rename inner test class to avoid name clash --- .../beam/sdk/options/PipelineOptionsReflectorTest.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/sdks/java/core/src/test/java/org/apache/beam/sdk/options/PipelineOptionsReflectorTest.java b/sdks/java/core/src/test/java/org/apache/beam/sdk/options/PipelineOptionsReflectorTest.java index bb2fedf645ba..82f032963a7d 100644 --- a/sdks/java/core/src/test/java/org/apache/beam/sdk/options/PipelineOptionsReflectorTest.java +++ b/sdks/java/core/src/test/java/org/apache/beam/sdk/options/PipelineOptionsReflectorTest.java @@ -118,13 +118,13 @@ interface ExtendsNonPipelineOptions extends NoExtendsClause, PipelineOptions {} @Test public void testExcludesHiddenInterfaces() { Set properties = - PipelineOptionsReflector.getOptionSpecs(Hidden.class); + PipelineOptionsReflector.getOptionSpecs(HiddenOptions.class); assertThat(properties, not(hasItem(hasName("foo")))); } - @org.apache.beam.sdk.options.Hidden - interface Hidden extends PipelineOptions { + @Hidden + interface HiddenOptions extends PipelineOptions { String getFoo(); void setFoo(String value); } From 3c35be7edf87f33f1ab72a5e5b8027b6caff228e Mon Sep 17 00:00:00 2001 From: Scott Wegner Date: Thu, 21 Apr 2016 15:02:14 -0700 Subject: [PATCH 11/13] fixup! Add explicit Nullable annotation to AutoValue type --- .../apache/beam/sdk/options/ProxyInvocationHandler.java | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/sdks/java/core/src/main/java/org/apache/beam/sdk/options/ProxyInvocationHandler.java b/sdks/java/core/src/main/java/org/apache/beam/sdk/options/ProxyInvocationHandler.java index f0b3f083d271..9a47be32e9a9 100644 --- a/sdks/java/core/src/main/java/org/apache/beam/sdk/options/ProxyInvocationHandler.java +++ b/sdks/java/core/src/main/java/org/apache/beam/sdk/options/ProxyInvocationHandler.java @@ -67,6 +67,7 @@ import java.util.Set; import java.util.SortedMap; import java.util.TreeMap; +import javax.annotation.Nullable; import javax.annotation.concurrent.ThreadSafe; /** @@ -169,24 +170,26 @@ public Object invoke(Object proxy, Method method, Object[] args) { */ @AutoValue abstract static class BoundValue { + @Nullable abstract Object getValue(); + abstract boolean isDefault(); - private static BoundValue of(Object value, boolean isDefault) { + private static BoundValue of(@Nullable Object value, boolean isDefault) { return new AutoValue_ProxyInvocationHandler_BoundValue(value, isDefault); } /** * Create a {@link BoundValue} representing an explicitly set option. */ - static BoundValue fromExplicitOption(Object value) { + static BoundValue fromExplicitOption(@Nullable Object value) { return BoundValue.of(value, false); } /** * Create a {@link BoundValue} representing a default option value. */ - static BoundValue fromDefault(Object value) { + static BoundValue fromDefault(@Nullable Object value) { return BoundValue.of(value, true); } } From b6fc084140a837276cafe42e2166250246c2549a Mon Sep 17 00:00:00 2001 From: Scott Wegner Date: Thu, 21 Apr 2016 15:47:09 -0700 Subject: [PATCH 12/13] fixup! Remove trailing whitespace --- .../org/apache/beam/sdk/options/ProxyInvocationHandler.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sdks/java/core/src/main/java/org/apache/beam/sdk/options/ProxyInvocationHandler.java b/sdks/java/core/src/main/java/org/apache/beam/sdk/options/ProxyInvocationHandler.java index 9a47be32e9a9..e1ea561492a1 100644 --- a/sdks/java/core/src/main/java/org/apache/beam/sdk/options/ProxyInvocationHandler.java +++ b/sdks/java/core/src/main/java/org/apache/beam/sdk/options/ProxyInvocationHandler.java @@ -172,7 +172,7 @@ public Object invoke(Object proxy, Method method, Object[] args) { abstract static class BoundValue { @Nullable abstract Object getValue(); - + abstract boolean isDefault(); private static BoundValue of(@Nullable Object value, boolean isDefault) { From 45aa73c14326ded9126cc021c5cef78eebb4efbd Mon Sep 17 00:00:00 2001 From: Scott Wegner Date: Fri, 22 Apr 2016 14:34:22 -0700 Subject: [PATCH 13/13] fixup! improve javadoc comment --- .../apache/beam/sdk/options/ProxyInvocationHandler.java | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/sdks/java/core/src/main/java/org/apache/beam/sdk/options/ProxyInvocationHandler.java b/sdks/java/core/src/main/java/org/apache/beam/sdk/options/ProxyInvocationHandler.java index e1ea561492a1..a269f4e6f8b7 100644 --- a/sdks/java/core/src/main/java/org/apache/beam/sdk/options/ProxyInvocationHandler.java +++ b/sdks/java/core/src/main/java/org/apache/beam/sdk/options/ProxyInvocationHandler.java @@ -320,12 +320,11 @@ private void populateDisplayData(DisplayData.Builder builder) { /** * Marker interface used when the original {@link PipelineOptions} interface is not known at - * runtime. This can occur when {@link PipelineOptions} are deserialized from JSON or specified - * on the command line. + * runtime. This can occur if {@link PipelineOptions} are deserialized from JSON. * *

    Pipeline authors can ensure {@link PipelineOptions} type information is available at - * runtime by explicitly registering {@link PipelineOptions options} interfaces via - * {@link PipelineOptionsFactory#register(Class)}. + * runtime by registering their {@link PipelineOptions options} interfaces. See the "Registration" + * section of {@link PipelineOptions} documentation. */ interface UnknownPipelineOptions extends PipelineOptions {}