From cbc323b2eb929cacf9f35be08d4c506e6580e705 Mon Sep 17 00:00:00 2001 From: Brian Hulette Date: Thu, 25 Feb 2021 14:24:58 -0800 Subject: [PATCH 1/2] Re-raise underlying exception for InvocationTargetException --- .../runners/dataflow/DataflowRunnerTest.java | 27 ++++++------------- .../apache/beam/sdk/util/InstanceBuilder.java | 16 ++++++++--- 2 files changed, 21 insertions(+), 22 deletions(-) diff --git a/runners/google-cloud-dataflow-java/src/test/java/org/apache/beam/runners/dataflow/DataflowRunnerTest.java b/runners/google-cloud-dataflow-java/src/test/java/org/apache/beam/runners/dataflow/DataflowRunnerTest.java index c79f7919c95a..8e0172150b71 100644 --- a/runners/google-cloud-dataflow-java/src/test/java/org/apache/beam/runners/dataflow/DataflowRunnerTest.java +++ b/runners/google-cloud-dataflow-java/src/test/java/org/apache/beam/runners/dataflow/DataflowRunnerTest.java @@ -20,7 +20,6 @@ import static org.apache.beam.runners.dataflow.DataflowRunner.getContainerImageForJob; import static org.apache.beam.vendor.guava.v26_0_jre.com.google.common.io.Files.getFileExtension; import static org.hamcrest.MatcherAssert.assertThat; -import static org.hamcrest.Matchers.both; import static org.hamcrest.Matchers.containsInAnyOrder; import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.endsWith; @@ -28,6 +27,7 @@ import static org.hamcrest.Matchers.hasEntry; import static org.hamcrest.Matchers.hasItem; import static org.hamcrest.Matchers.hasKey; +import static org.hamcrest.Matchers.hasProperty; import static org.hamcrest.Matchers.is; import static org.hamcrest.Matchers.lessThanOrEqualTo; import static org.hamcrest.Matchers.not; @@ -153,7 +153,6 @@ import org.apache.beam.sdk.values.TimestampedValue; import org.apache.beam.sdk.values.WindowingStrategy; import org.apache.beam.vendor.grpc.v1p26p0.com.google.protobuf.InvalidProtocolBufferException; -import org.apache.beam.vendor.guava.v26_0_jre.com.google.common.base.Throwables; import org.apache.beam.vendor.guava.v26_0_jre.com.google.common.collect.ImmutableList; import org.apache.beam.vendor.guava.v26_0_jre.com.google.common.collect.Iterables; import org.checkerframework.checker.nullness.qual.Nullable; @@ -351,14 +350,9 @@ public void testPathValidation() { "--credentialFactoryClass=" + NoopCredentialFactory.class.getName(), }; - try { - Pipeline.create(PipelineOptionsFactory.fromArgs(args).create()).run(); - fail(); - } catch (RuntimeException e) { - assertThat( - Throwables.getStackTraceAsString(e), - containsString("DataflowRunner requires gcpTempLocation")); - } + thrown.expect(IllegalArgumentException.class); + thrown.expectMessage("DataflowRunner requires gcpTempLocation"); + Pipeline.create(PipelineOptionsFactory.fromArgs(args).create()).run(); } @Test @@ -372,15 +366,10 @@ public void testPathExistsValidation() { "--credentialFactoryClass=" + NoopCredentialFactory.class.getName(), }; - try { - Pipeline.create(PipelineOptionsFactory.fromArgs(args).create()).run(); - fail(); - } catch (RuntimeException e) { - assertThat( - Throwables.getStackTraceAsString(e), - both(containsString("gs://does/not/exist")) - .and(containsString("Unable to verify that GCS bucket gs://does exists"))); - } + thrown.expect(IllegalArgumentException.class); + thrown.expectMessage("gcpTempLocation"); + thrown.expectCause(hasProperty("message", containsString("gs://does/not/exist"))); + Pipeline.create(PipelineOptionsFactory.fromArgs(args).create()).run(); } @Test diff --git a/sdks/java/core/src/main/java/org/apache/beam/sdk/util/InstanceBuilder.java b/sdks/java/core/src/main/java/org/apache/beam/sdk/util/InstanceBuilder.java index f6d9ffe2277d..191b71436741 100644 --- a/sdks/java/core/src/main/java/org/apache/beam/sdk/util/InstanceBuilder.java +++ b/sdks/java/core/src/main/java/org/apache/beam/sdk/util/InstanceBuilder.java @@ -221,11 +221,21 @@ private T buildFromMethod(Class[] types) { String.format( "Unable to find factory method %s#%s(%s)", factoryClass.getSimpleName(), methodName, Joiner.on(", ").join(types))); - - } catch (IllegalAccessException | InvocationTargetException e) { + } catch (InvocationTargetException e) { + if (e.getTargetException() instanceof RuntimeException) { + // If underlying exception is unchecked re-raise it as-is + throw (RuntimeException) e.getTargetException(); + } else { + throw new RuntimeException( + String.format( + "Encountered checked exception when constructing an instance from factory method %s#%s(%s)", + factoryClass.getSimpleName(), methodName, Joiner.on(", ").join(types)), + e.getTargetException()); + } + } catch (IllegalAccessException e) { throw new RuntimeException( String.format( - "Failed to construct instance from factory method %s#%s(%s)", + "Failed to construct instance from factory method %s#%s(%s) due to access restriction", factoryClass.getSimpleName(), methodName, Joiner.on(", ").join(types)), e); } From b9088f5adb827c0e4569adad6a9e4516012bab29 Mon Sep 17 00:00:00 2001 From: Brian Hulette Date: Thu, 25 Feb 2021 16:00:57 -0800 Subject: [PATCH 2/2] remove unnecessary else --- .../org/apache/beam/sdk/util/InstanceBuilder.java | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/sdks/java/core/src/main/java/org/apache/beam/sdk/util/InstanceBuilder.java b/sdks/java/core/src/main/java/org/apache/beam/sdk/util/InstanceBuilder.java index 191b71436741..1639186a599d 100644 --- a/sdks/java/core/src/main/java/org/apache/beam/sdk/util/InstanceBuilder.java +++ b/sdks/java/core/src/main/java/org/apache/beam/sdk/util/InstanceBuilder.java @@ -225,13 +225,12 @@ private T buildFromMethod(Class[] types) { if (e.getTargetException() instanceof RuntimeException) { // If underlying exception is unchecked re-raise it as-is throw (RuntimeException) e.getTargetException(); - } else { - throw new RuntimeException( - String.format( - "Encountered checked exception when constructing an instance from factory method %s#%s(%s)", - factoryClass.getSimpleName(), methodName, Joiner.on(", ").join(types)), - e.getTargetException()); } + throw new RuntimeException( + String.format( + "Encountered checked exception when constructing an instance from factory method %s#%s(%s)", + factoryClass.getSimpleName(), methodName, Joiner.on(", ").join(types)), + e.getTargetException()); } catch (IllegalAccessException e) { throw new RuntimeException( String.format(