[BEAM-3202] Ensure that PipelineOptions.getOptionsId is always populated.
Project: http://git-wip-us.apache.org/repos/asf/beam/repo Commit: http://git-wip-us.apache.org/repos/asf/beam/commit/c234352e Tree: http://git-wip-us.apache.org/repos/asf/beam/tree/c234352e Diff: http://git-wip-us.apache.org/repos/asf/beam/diff/c234352e Branch: refs/heads/tez-runner Commit: c234352e4a0d159724b4c6ef339e82b43b7026c6 Parents: f96bbd4 Author: Luke Cwik <lc...@google.com> Authored: Thu Nov 16 09:40:42 2017 -0800 Committer: Luke Cwik <lc...@google.com> Committed: Thu Nov 16 14:28:10 2017 -0800 ---------------------------------------------------------------------- .../org/apache/beam/sdk/options/PipelineOptions.java | 2 +- .../beam/sdk/options/PipelineOptionsFactory.java | 4 ++++ .../beam/sdk/options/PipelineOptionsFactoryTest.java | 15 +++++++++++++++ .../apache/beam/sdk/testing/TestPipelineTest.java | 6 ++++-- 4 files changed, 24 insertions(+), 3 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/beam/blob/c234352e/sdks/java/core/src/main/java/org/apache/beam/sdk/options/PipelineOptions.java ---------------------------------------------------------------------- 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 77117b6..1dc9d44 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 @@ -334,7 +334,7 @@ public interface PipelineOptions extends HasDisplayData { Map<String, Map<String, Object>> outputRuntimeOptions(); /** - * Provides a unique ID for this {@link PipelineOptions} object, assigned at graph + * Provides a process wide unique ID for this {@link PipelineOptions} object, assigned at graph * construction time. */ @Hidden http://git-wip-us.apache.org/repos/asf/beam/blob/c234352e/sdks/java/core/src/main/java/org/apache/beam/sdk/options/PipelineOptionsFactory.java ---------------------------------------------------------------------- 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 ad6979e..f75daee 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 @@ -302,6 +302,10 @@ public class PipelineOptionsFactory { appNameOptions.setAppName(defaultAppName); } + // Ensure the options id has been populated either by the user using the command line + // or by the default value factory. + t.getOptionsId(); + if (validation) { if (isCli) { PipelineOptionsValidator.validateCli(klass, t); http://git-wip-us.apache.org/repos/asf/beam/blob/c234352e/sdks/java/core/src/test/java/org/apache/beam/sdk/options/PipelineOptionsFactoryTest.java ---------------------------------------------------------------------- diff --git a/sdks/java/core/src/test/java/org/apache/beam/sdk/options/PipelineOptionsFactoryTest.java b/sdks/java/core/src/test/java/org/apache/beam/sdk/options/PipelineOptionsFactoryTest.java index f8de74a..b534ed8 100644 --- a/sdks/java/core/src/test/java/org/apache/beam/sdk/options/PipelineOptionsFactoryTest.java +++ b/sdks/java/core/src/test/java/org/apache/beam/sdk/options/PipelineOptionsFactoryTest.java @@ -36,6 +36,7 @@ import com.fasterxml.jackson.databind.DeserializationContext; import com.fasterxml.jackson.databind.JsonDeserializer; import com.fasterxml.jackson.databind.JsonSerializer; import com.fasterxml.jackson.databind.Module; +import com.fasterxml.jackson.databind.ObjectMapper; import com.fasterxml.jackson.databind.SerializerProvider; import com.fasterxml.jackson.databind.annotation.JsonDeserialize; import com.fasterxml.jackson.databind.annotation.JsonSerialize; @@ -58,6 +59,7 @@ import org.apache.beam.sdk.runners.PipelineRunnerRegistrar; import org.apache.beam.sdk.testing.CrashingRunner; import org.apache.beam.sdk.testing.ExpectedLogs; import org.apache.beam.sdk.testing.RestoreSystemProperties; +import org.apache.beam.sdk.util.common.ReflectHelpers; import org.hamcrest.Matchers; import org.junit.Rule; import org.junit.Test; @@ -123,6 +125,19 @@ public class PipelineOptionsFactoryTest { } @Test + public void testOptionsIdIsSet() throws Exception { + ObjectMapper mapper = new ObjectMapper().registerModules( + ObjectMapper.findModules(ReflectHelpers.findClassLoader())); + PipelineOptions options = PipelineOptionsFactory.create(); + // We purposely serialize/deserialize to get another instance. This allows to test if the + // default has been set or not. + PipelineOptions clone = + mapper.readValue(mapper.writeValueAsString(options), PipelineOptions.class); + // It is important that we don't call getOptionsId() before we have created the clone. + assertEquals(options.getOptionsId(), clone.getOptionsId()); + } + + @Test public void testManualRegistration() { assertFalse(PipelineOptionsFactory.getRegisteredOptions().contains(TestPipelineOptions.class)); PipelineOptionsFactory.register(TestPipelineOptions.class); http://git-wip-us.apache.org/repos/asf/beam/blob/c234352e/sdks/java/core/src/test/java/org/apache/beam/sdk/testing/TestPipelineTest.java ---------------------------------------------------------------------- diff --git a/sdks/java/core/src/test/java/org/apache/beam/sdk/testing/TestPipelineTest.java b/sdks/java/core/src/test/java/org/apache/beam/sdk/testing/TestPipelineTest.java index ec681ea..8d50c59 100644 --- a/sdks/java/core/src/test/java/org/apache/beam/sdk/testing/TestPipelineTest.java +++ b/sdks/java/core/src/test/java/org/apache/beam/sdk/testing/TestPipelineTest.java @@ -125,10 +125,12 @@ public class TestPipelineTest implements Serializable { PipelineOptions options = PipelineOptionsFactory.fromArgs(args).as(PipelineOptions.class); String[] arr = TestPipeline.convertToArgs(options); List<String> lst = Arrays.asList(arr); - assertEquals(lst.size(), 2); + assertEquals(lst.size(), 3); assertThat( lst, - containsInAnyOrder("--tempLocation=Test_Location", "--appName=TestPipelineCreationTest")); + containsInAnyOrder("--tempLocation=Test_Location", + "--appName=TestPipelineCreationTest", + "--optionsId=" + options.getOptionsId())); } @Test