From b41c30b870b7606a8ed48c679123a92e6b3a2423 Mon Sep 17 00:00:00 2001 From: Kyle Bendickson Date: Tue, 5 Jul 2022 13:21:55 -0700 Subject: [PATCH 1/5] API - Deprecate unused runSafely functions and remove their associated tests --- .../apache/iceberg/util/ExceptionUtil.java | 4 + .../iceberg/util/TestExceptionUtil.java | 170 ------------------ 2 files changed, 4 insertions(+), 170 deletions(-) delete mode 100644 api/src/test/java/org/apache/iceberg/util/TestExceptionUtil.java diff --git a/api/src/main/java/org/apache/iceberg/util/ExceptionUtil.java b/api/src/main/java/org/apache/iceberg/util/ExceptionUtil.java index 76a1a1c97c4f..fa424e692391 100644 --- a/api/src/main/java/org/apache/iceberg/util/ExceptionUtil.java +++ b/api/src/main/java/org/apache/iceberg/util/ExceptionUtil.java @@ -53,6 +53,7 @@ public interface FinallyBlock { void run() throws Exception; } + @Deprecated public static R runSafely( Block block, CatchBlock catchBlock, @@ -61,6 +62,7 @@ public static R runSafely( RuntimeException.class, RuntimeException.class, RuntimeException.class); } + @Deprecated public static R runSafely( Block block, CatchBlock catchBlock, @@ -69,6 +71,7 @@ public static R runSafely( return runSafely(block, catchBlock, finallyBlock, e1Class, RuntimeException.class, RuntimeException.class); } + @Deprecated public static R runSafely( Block block, CatchBlock catchBlock, @@ -78,6 +81,7 @@ public static R runSafely( return runSafely(block, catchBlock, finallyBlock, e1Class, e2Class, RuntimeException.class); } + @Deprecated public static R runSafely( Block block, CatchBlock catchBlock, diff --git a/api/src/test/java/org/apache/iceberg/util/TestExceptionUtil.java b/api/src/test/java/org/apache/iceberg/util/TestExceptionUtil.java deleted file mode 100644 index b02717dc33d4..000000000000 --- a/api/src/test/java/org/apache/iceberg/util/TestExceptionUtil.java +++ /dev/null @@ -1,170 +0,0 @@ -/* - * 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.iceberg.util; - -import java.io.IOException; -import org.assertj.core.api.Assertions; -import org.junit.Assert; -import org.junit.Test; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; - -public class TestExceptionUtil { - private static final Logger LOG = LoggerFactory.getLogger(TestExceptionUtil.class); - - private static class CustomCheckedException extends Exception { - private CustomCheckedException(String message) { - super(message); - } - } - - @Test - public void testRunSafely() { - CustomCheckedException exc = new CustomCheckedException("test"); - try { - ExceptionUtil.runSafely(() -> { - throw exc; - }, e -> { - throw new Exception("test catch suppression"); - }, () -> { - throw new RuntimeException("test finally suppression"); - }, CustomCheckedException.class - ); - - Assert.fail("Should have thrown CustomCheckedException"); - - } catch (CustomCheckedException e) { - LOG.info("Final exception", e); - Assert.assertEquals("Should throw correct exception instance", exc, e); - Assert.assertEquals("Should not alter exception message", "test", e.getMessage()); - Assert.assertEquals("Should have 2 suppressed exceptions", 2, e.getSuppressed().length); - - Throwable throwSuppressed = e.getSuppressed()[0]; - Assertions.assertThat(throwSuppressed).as("Should be an Exception").isInstanceOf(Exception.class); - Assert.assertEquals("Should have correct message", "test catch suppression", throwSuppressed.getMessage()); - - Throwable finallySuppressed = e.getSuppressed()[1]; - Assertions.assertThat(finallySuppressed).as("Should be a RuntimeException").isInstanceOf(RuntimeException.class); - Assert.assertEquals("Should have correct message", "test finally suppression", finallySuppressed.getMessage()); - } - } - - @Test - public void testRunSafelyTwoExceptions() { - CustomCheckedException exc = new CustomCheckedException("test"); - try { - ExceptionUtil.runSafely((ExceptionUtil.Block) () -> { - throw exc; - }, e -> { - throw new Exception("test catch suppression"); - }, () -> { - throw new RuntimeException("test finally suppression"); - }, CustomCheckedException.class, IOException.class - ); - - Assert.fail("Should have thrown CustomCheckedException"); - - } catch (IOException e) { - Assert.fail("Should not have thrown exception class: " + e.getClass().getName()); - - } catch (CustomCheckedException e) { - LOG.info("Final exception", e); - Assert.assertEquals("Should throw correct exception instance", exc, e); - Assert.assertEquals("Should not alter exception message", "test", e.getMessage()); - Assert.assertEquals("Should have 2 suppressed exceptions", 2, e.getSuppressed().length); - - Throwable throwSuppressed = e.getSuppressed()[0]; - Assertions.assertThat(throwSuppressed).as("Should be an Exception").isInstanceOf(Exception.class); - Assert.assertEquals("Should have correct message", "test catch suppression", throwSuppressed.getMessage()); - - Throwable finallySuppressed = e.getSuppressed()[1]; - Assertions.assertThat(finallySuppressed).as("Should be a RuntimeException").isInstanceOf(RuntimeException.class); - Assert.assertEquals("Should have correct message", "test finally suppression", finallySuppressed.getMessage()); - } - } - - @Test - public void testRunSafelyThreeExceptions() { - CustomCheckedException exc = new CustomCheckedException("test"); - try { - ExceptionUtil.runSafely((ExceptionUtil.Block) - () -> { - throw exc; - }, e -> { - throw new Exception("test catch suppression"); - }, () -> { - throw new RuntimeException("test finally suppression"); - }, CustomCheckedException.class, IOException.class, ClassNotFoundException.class - ); - - Assert.fail("Should have thrown CustomCheckedException"); - - } catch (IOException | ClassNotFoundException e) { - Assert.fail("Should not have thrown exception class: " + e.getClass().getName()); - - } catch (CustomCheckedException e) { - LOG.info("Final exception", e); - Assert.assertEquals("Should throw correct exception instance", exc, e); - Assert.assertEquals("Should not alter exception message", "test", e.getMessage()); - Assert.assertEquals("Should have 2 suppressed exceptions", 2, e.getSuppressed().length); - - Throwable throwSuppressed = e.getSuppressed()[0]; - Assertions.assertThat(throwSuppressed).as("Should be an Exception").isInstanceOf(Exception.class); - Assert.assertEquals("Should have correct message", "test catch suppression", throwSuppressed.getMessage()); - - Throwable finallySuppressed = e.getSuppressed()[1]; - Assertions.assertThat(finallySuppressed).as("Should be a RuntimeException").isInstanceOf(RuntimeException.class); - Assert.assertEquals("Should have correct message", "test finally suppression", finallySuppressed.getMessage()); - } - } - - @Test - public void testRunSafelyRuntimeExceptions() { - RuntimeException exc = new RuntimeException("test"); - try { - ExceptionUtil.runSafely(() -> { - throw exc; - }, e -> { - throw new Exception("test catch suppression"); - }, () -> { - throw new CustomCheckedException("test finally suppression"); - } - ); - - Assert.fail("Should have thrown RuntimeException"); - - } catch (RuntimeException e) { - LOG.info("Final exception", e); - Assert.assertEquals("Should throw correct exception instance", exc, e); - Assert.assertEquals("Should not alter exception message", "test", e.getMessage()); - Assert.assertEquals("Should have 2 suppressed exceptions", 2, e.getSuppressed().length); - - Throwable throwSuppressed = e.getSuppressed()[0]; - Assertions.assertThat(throwSuppressed).as("Should be an Exception").isInstanceOf(Exception.class); - Assert.assertEquals("Should have correct message", "test catch suppression", throwSuppressed.getMessage()); - - Throwable finallySuppressed = e.getSuppressed()[1]; - Assertions.assertThat(finallySuppressed).as("Should be a CustomCheckedException") - .isInstanceOf(CustomCheckedException.class); - Assert.assertEquals("Should have correct message", "test finally suppression", finallySuppressed.getMessage()); - } - } - -} From 018245a67f3e5e03d17b122634a90208ad401594 Mon Sep 17 00:00:00 2001 From: Kyle Bendickson Date: Tue, 5 Jul 2022 13:31:17 -0700 Subject: [PATCH 2/5] Add a suppression for error prone for the unused / deprecated function runSafely --- api/src/main/java/org/apache/iceberg/util/ExceptionUtil.java | 1 + 1 file changed, 1 insertion(+) diff --git a/api/src/main/java/org/apache/iceberg/util/ExceptionUtil.java b/api/src/main/java/org/apache/iceberg/util/ExceptionUtil.java index fa424e692391..8be641535e86 100644 --- a/api/src/main/java/org/apache/iceberg/util/ExceptionUtil.java +++ b/api/src/main/java/org/apache/iceberg/util/ExceptionUtil.java @@ -82,6 +82,7 @@ public static R runSafely( } @Deprecated + @SuppressWarnings("Finally") public static R runSafely( Block block, CatchBlock catchBlock, From 870563c515ee21360e8d7705c30da4857f7bd8b2 Mon Sep 17 00:00:00 2001 From: Kyle Bendickson Date: Tue, 5 Jul 2022 14:06:40 -0700 Subject: [PATCH 3/5] Add TestExceptionUtil back in --- .../iceberg/util/TestExceptionUtil.java | 171 ++++++++++++++++++ 1 file changed, 171 insertions(+) create mode 100644 api/src/test/java/org/apache/iceberg/util/TestExceptionUtil.java diff --git a/api/src/test/java/org/apache/iceberg/util/TestExceptionUtil.java b/api/src/test/java/org/apache/iceberg/util/TestExceptionUtil.java new file mode 100644 index 000000000000..adbc7f0b1e84 --- /dev/null +++ b/api/src/test/java/org/apache/iceberg/util/TestExceptionUtil.java @@ -0,0 +1,171 @@ +/* + * 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.iceberg.util; + +import java.io.IOException; +import org.assertj.core.api.Assertions; +import org.junit.Assert; +import org.junit.Test; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +public class TestExceptionUtil { + private static final Logger LOG = LoggerFactory.getLogger(TestExceptionUtil.class); + + private static class CustomCheckedException extends Exception { + private CustomCheckedException(String message) { + super(message); + } + } + + @Test + public void testRunSafely() { + CustomCheckedException exc = new CustomCheckedException("test"); + try { + ExceptionUtil.runSafely(() -> { + throw exc; + }, e -> { + throw new Exception("test catch suppression"); + }, () -> { + throw new RuntimeException("test finally suppression"); + }, CustomCheckedException.class + ); + + Assert.fail("Should have thrown CustomCheckedException"); + + } catch (CustomCheckedException e) { + LOG.info("Final exception", e); + Assert.assertEquals("Should throw correct exception instance", exc, e); + Assert.assertEquals("Should not alter exception message", "test", e.getMessage()); + Assert.assertEquals("Should have 2 suppressed exceptions", 2, e.getSuppressed().length); + + Throwable throwSuppressed = e.getSuppressed()[0]; + Assertions.assertThat(throwSuppressed).as("Should be an Exception").isInstanceOf(Exception.class); + Assert.assertEquals("Should have correct message", "test catch suppression", throwSuppressed.getMessage()); + + Throwable finallySuppressed = e.getSuppressed()[1]; + Assertions.assertThat(finallySuppressed).as("Should be a RuntimeException").isInstanceOf(RuntimeException.class); + Assert.assertEquals("Should have correct message", "test finally suppression", finallySuppressed.getMessage()); + } + } + + @Test + public void testRunSafelyTwoExceptions() { + CustomCheckedException exc = new CustomCheckedException("test"); + try { + ExceptionUtil.runSafely((ExceptionUtil.Block) () -> { + throw exc; + }, e -> { + throw new Exception("test catch suppression"); + }, () -> { + throw new RuntimeException("test finally suppression"); + }, CustomCheckedException.class, IOException.class + ); + + Assert.fail("Should have thrown CustomCheckedException"); + + } catch (IOException e) { + Assert.fail("Should not have thrown exception class: " + e.getClass().getName()); + + } catch (CustomCheckedException e) { + LOG.info("Final exception", e); + Assert.assertEquals("Should throw correct exception instance", exc, e); + Assert.assertEquals("Should not alter exception message", "test", e.getMessage()); + Assert.assertEquals("Should have 2 suppressed exceptions", 2, e.getSuppressed().length); + + Throwable throwSuppressed = e.getSuppressed()[0]; + Assertions.assertThat(throwSuppressed).as("Should be an Exception").isInstanceOf(Exception.class); + Assert.assertEquals("Should have correct message", "test catch suppression", throwSuppressed.getMessage()); + + Throwable finallySuppressed = e.getSuppressed()[1]; + Assertions.assertThat(finallySuppressed).as("Should be a RuntimeException").isInstanceOf(RuntimeException.class); + Assert.assertEquals("Should have correct message", "test finally suppression", finallySuppressed.getMessage()); + } + } + + @Test + public void testRunSafelyThreeExceptions() { + CustomCheckedException exc = new CustomCheckedException("test"); + try { + ExceptionUtil.runSafely((ExceptionUtil.Block) + () -> { + throw exc; + }, e -> { + throw new Exception("test catch suppression"); + }, () -> { + throw new RuntimeException("test finally suppression"); + }, CustomCheckedException.class, IOException.class, ClassNotFoundException.class + ); + + Assert.fail("Should have thrown CustomCheckedException"); + + } catch (IOException | ClassNotFoundException e) { + Assert.fail("Should not have thrown exception class: " + e.getClass().getName()); + + } catch (CustomCheckedException e) { + LOG.info("Final exception", e); + Assert.assertEquals("Should throw correct exception instance", exc, e); + Assert.assertEquals("Should not alter exception message", "test", e.getMessage()); + Assert.assertEquals("Should have 2 suppressed exceptions", 2, e.getSuppressed().length); + + Throwable throwSuppressed = e.getSuppressed()[0]; + Assertions.assertThat(throwSuppressed).as("Should be an Exception").isInstanceOf(Exception.class); + Assert.assertEquals("Should have correct message", "test catch suppression", throwSuppressed.getMessage()); + + Throwable finallySuppressed = e.getSuppressed()[1]; + Assertions.assertThat(finallySuppressed).as("Should be a RuntimeException").isInstanceOf(RuntimeException.class); + Assert.assertEquals("Should have correct message", "test finally suppression", finallySuppressed.getMessage()); + } + } + + @Test + public void testRunSafelyRuntimeExceptions() { + RuntimeException exc = new RuntimeException("test"); + try { + ExceptionUtil.runSafely(() -> { + throw exc; + }, e -> { + throw new Exception("test catch suppression"); + }, () -> { + throw new CustomCheckedException("test finally suppression"); + } + ); + + Assert.fail("Should have thrown RuntimeException"); + + } catch (RuntimeException e) { + LOG.info("Final exception", e); + Assert.assertEquals("Should throw correct exception instance", exc, e); + Assert.assertEquals("Should not alter exception message", "test", e.getMessage()); + Assert.assertEquals("Should have 2 suppressed exceptions", 2, e.getSuppressed().length); + + Throwable throwSuppressed = e.getSuppressed()[0]; + Assertions.assertThat(throwSuppressed).as("Should be an Exception").isInstanceOf(Exception.class); + Assert.assertEquals("Should have correct message", "test catch suppression", throwSuppressed.getMessage()); + + Throwable finallySuppressed = e.getSuppressed()[1]; + Assertions.assertThat(finallySuppressed).as("Should be a CustomCheckedException") + .isInstanceOf(CustomCheckedException.class); + Assert.assertEquals("Should have correct message", "test finally suppression", finallySuppressed.getMessage()); + } + } + +} + From e809ababc3fd092edf47787184ec22d97cb61917 Mon Sep 17 00:00:00 2001 From: Kyle Bendickson Date: Tue, 5 Jul 2022 14:07:21 -0700 Subject: [PATCH 4/5] Remove added newline --- api/src/test/java/org/apache/iceberg/util/TestExceptionUtil.java | 1 - 1 file changed, 1 deletion(-) diff --git a/api/src/test/java/org/apache/iceberg/util/TestExceptionUtil.java b/api/src/test/java/org/apache/iceberg/util/TestExceptionUtil.java index adbc7f0b1e84..b02717dc33d4 100644 --- a/api/src/test/java/org/apache/iceberg/util/TestExceptionUtil.java +++ b/api/src/test/java/org/apache/iceberg/util/TestExceptionUtil.java @@ -168,4 +168,3 @@ public void testRunSafelyRuntimeExceptions() { } } - From 749e56b3a3739310837f4b3a1360f66af00bb90f Mon Sep 17 00:00:00 2001 From: Kyle Bendickson Date: Tue, 5 Jul 2022 20:29:21 -0700 Subject: [PATCH 5/5] Add @deprecated javadoc tag indicating when these functions will be removed and mentioning the unit test so we don't forget to remove those tooo --- .../org/apache/iceberg/util/ExceptionUtil.java | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/api/src/main/java/org/apache/iceberg/util/ExceptionUtil.java b/api/src/main/java/org/apache/iceberg/util/ExceptionUtil.java index 8be641535e86..f6301aec1c76 100644 --- a/api/src/main/java/org/apache/iceberg/util/ExceptionUtil.java +++ b/api/src/main/java/org/apache/iceberg/util/ExceptionUtil.java @@ -53,6 +53,10 @@ public interface FinallyBlock { void run() throws Exception; } + /** + * This function is unused outside of the associated tests in TestExceptionUtil. + * @deprecated since 0.14.0. Will be removed in 1.0.0. + */ @Deprecated public static R runSafely( Block block, @@ -62,6 +66,10 @@ public static R runSafely( RuntimeException.class, RuntimeException.class, RuntimeException.class); } + /** + * This function is unused outside of the associated tests in TestExceptionUtil. + * @deprecated since 0.14.0. Will be removed in 1.0.0. + */ @Deprecated public static R runSafely( Block block, @@ -71,6 +79,10 @@ public static R runSafely( return runSafely(block, catchBlock, finallyBlock, e1Class, RuntimeException.class, RuntimeException.class); } + /** + * This function is unused outside the associated tests in TestExceptionUtil. + * @deprecated since 0.14.0. Will be removed in 1.0.0. + */ @Deprecated public static R runSafely( Block block, @@ -81,6 +93,10 @@ public static R runSafely( return runSafely(block, catchBlock, finallyBlock, e1Class, e2Class, RuntimeException.class); } + /** + * This function is unused outside the associated tests in TestExceptionUtil. + * @deprecated since 0.14.0. Will be removed in 1.0.0. + */ @Deprecated @SuppressWarnings("Finally") public static R runSafely(