From d1389e65c11011ae2ce4d367df848edb7666aced Mon Sep 17 00:00:00 2001 From: Sadanand Shenoy Date: Wed, 9 Oct 2024 18:19:01 +0530 Subject: [PATCH] address comments --- .../org/apache/hadoop/hdds/HddsUtils.java | 18 +---------------- .../apache/ozone/test/GenericTestUtils.java | 15 ++++++++++++++ .../dev-support/checks/_mvn_unit_report.sh | 2 +- .../fs/ozone/AbstractOzoneFileSystemTest.java | 15 +++----------- .../ozone/client/rpc/OzoneRpcClientTests.java | 20 +++++++++---------- .../fs/ozone/BasicOzoneClientAdapterImpl.java | 2 +- .../BasicRootedOzoneClientAdapterImpl.java | 4 ++-- 7 files changed, 33 insertions(+), 43 deletions(-) diff --git a/hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/HddsUtils.java b/hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/HddsUtils.java index 60fc6252f31..31723647525 100644 --- a/hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/HddsUtils.java +++ b/hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/HddsUtils.java @@ -113,8 +113,6 @@ public final class HddsUtils { private static final int NO_PORT = -1; - private static boolean ignoreReportingLeak = false; - private HddsUtils() { } @@ -882,15 +880,8 @@ public static HddsProtos.UUID toProtobuf(UUID uuid) { : null; } - @VisibleForTesting - public static void setIgnoreReportingLeak(boolean ignoreReportingLeak) { - HddsUtils.ignoreReportingLeak = ignoreReportingLeak; - } - /** * Logs a warning to report that the class is not closed properly. - * If {@link HddsUtils#ignoreReportingLeak} is set to true it will log that - * the message has been ignored. This only serves for testing purposes. */ public static void reportLeak(Class clazz, String stackTrace, Logger log) { String warning = String.format("%s is not closed properly", clazz.getSimpleName()); @@ -899,13 +890,6 @@ public static void reportLeak(Class clazz, String stackTrace, Logger log) { stackTrace); warning = warning.concat(debugMessage); } - if (!ignoreReportingLeak) { - log.warn(warning); - } else { - String ignoreMessage = - String.format("Ignoring warning : %s is not closed correctly", - clazz.getSimpleName()); - log.warn(ignoreMessage); - } + log.warn(warning); } } diff --git a/hadoop-hdds/test-utils/src/main/java/org/apache/ozone/test/GenericTestUtils.java b/hadoop-hdds/test-utils/src/main/java/org/apache/ozone/test/GenericTestUtils.java index 9a3a5c7a8f1..48abd5e986e 100644 --- a/hadoop-hdds/test-utils/src/main/java/org/apache/ozone/test/GenericTestUtils.java +++ b/hadoop-hdds/test-utils/src/main/java/org/apache/ozone/test/GenericTestUtils.java @@ -29,6 +29,7 @@ import java.time.Instant; import java.util.List; import java.util.Map; +import java.util.concurrent.Callable; import java.util.concurrent.TimeoutException; import com.google.common.base.Preconditions; @@ -205,6 +206,20 @@ public static void waitFor(BooleanSupplier check, int checkEveryMillis, } } + public static T assertThrows( + Class expectedType, + Callable func) { + return Assertions.assertThrows(expectedType, () -> { + final AutoCloseable closeable = func.call(); + try { + if (closeable != null) { + closeable.close(); + } + } catch (Exception ignored) { + } + }); + } + /** * @deprecated use sl4fj based version */ diff --git a/hadoop-ozone/dev-support/checks/_mvn_unit_report.sh b/hadoop-ozone/dev-support/checks/_mvn_unit_report.sh index 44090eec312..36205c69bb6 100755 --- a/hadoop-ozone/dev-support/checks/_mvn_unit_report.sh +++ b/hadoop-ozone/dev-support/checks/_mvn_unit_report.sh @@ -37,7 +37,7 @@ find "." -not -path '*/iteration*' -name 'TEST*.xml' -print0 \ if [[ "${CHECK:-unit}" == "integration" ]]; then find hadoop-ozone/integration-test -not -path '*/iteration*' -name '*-output.txt' -print0 \ | xargs -n1 -0 "grep" -l -E "not closed properly|was not shutdown properly" \ - | awk -F/ '{sub("-output.txt",""); print $NF ": Memory Leak detected in test, Failing."}' \ + | awk -F/ '{sub("-output.txt",""); print $NF}' \ >> "${tempfile}" fi diff --git a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/AbstractOzoneFileSystemTest.java b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/AbstractOzoneFileSystemTest.java index 9df8e69ad44..e7c4cbee1d5 100644 --- a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/AbstractOzoneFileSystemTest.java +++ b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/AbstractOzoneFileSystemTest.java @@ -409,7 +409,7 @@ public void testCreateWithInvalidPaths() throws Exception { } private void checkInvalidPath(Path path) { - InvalidPathException pathException = assertThrows( + InvalidPathException pathException = GenericTestUtils.assertThrows( InvalidPathException.class, () -> fs.create(path, false) ); assertThat(pathException.getMessage()).contains("Invalid path Name"); @@ -2236,17 +2236,8 @@ void testFileSystemWithObjectStoreLayout() throws IOException { OzoneConfiguration config = new OzoneConfiguration(fs.getConf()); config.set(FS_DEFAULT_NAME_KEY, obsRootPath); - IllegalArgumentException e = - assertThrows(IllegalArgumentException.class, () -> { - FileSystem fileSystem = null; - try { - fileSystem = FileSystem.get(config); - } finally { - if (fileSystem != null) { - fileSystem.close(); - } - } - }); + IllegalArgumentException e = GenericTestUtils.assertThrows(IllegalArgumentException.class, + () -> FileSystem.get(config)); assertThat(e.getMessage()).contains("OBJECT_STORE, which does not support file system semantics"); } } diff --git a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/client/rpc/OzoneRpcClientTests.java b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/client/rpc/OzoneRpcClientTests.java index cc11f9715f7..77b0e5fe65e 100644 --- a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/client/rpc/OzoneRpcClientTests.java +++ b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/client/rpc/OzoneRpcClientTests.java @@ -191,6 +191,7 @@ import static org.junit.jupiter.api.Assertions.fail; import static org.slf4j.event.Level.DEBUG; +import org.apache.ozone.test.tag.Unhealthy; import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.MethodOrderer; import org.junit.jupiter.api.Test; @@ -264,12 +265,8 @@ static void startCluster(OzoneConfiguration conf, MiniOzoneCluster.Builder build /** * Close OzoneClient and shutdown MiniOzoneCluster. */ - static void shutdownCluster() throws IOException { - for (OzoneClient ozoneClient : ozoneClients) { - if (ozoneClient != null) { - ozoneClient.close(); - } - } + static void shutdownCluster() { + org.apache.hadoop.hdds.utils.IOUtils.closeQuietly(ozoneClients); ozoneClients.clear(); if (storageContainerLocationClient != null) { @@ -3150,9 +3147,13 @@ void testMultipartUploadOverride(ReplicationConfig replication) } - @Test + /** + * This test prints out that there is a memory leak in the test logs + * which during post-processing is caught by the CI thereby failing the + * CI run. Hence, disabling this for CI. + */ + @Unhealthy public void testClientLeakDetector() throws Exception { - HddsUtils.setIgnoreReportingLeak(true); OzoneClient client = OzoneClientFactory.getRpcClient(cluster.getConf()); String volumeName = UUID.randomUUID().toString(); String bucketName = UUID.randomUUID().toString(); @@ -3174,8 +3175,7 @@ public void testClientLeakDetector() throws Exception { client = null; System.gc(); GenericTestUtils.waitFor(() -> ozoneClientFactoryLogCapturer.getOutput() - .contains("is not closed correctly"), 100, 2000); - HddsUtils.setIgnoreReportingLeak(false); + .contains("is not closed properly"), 100, 2000); } @Test public void testMultipartUploadOwner() throws Exception { diff --git a/hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicOzoneClientAdapterImpl.java b/hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicOzoneClientAdapterImpl.java index b10b7af225b..618a837b168 100644 --- a/hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicOzoneClientAdapterImpl.java +++ b/hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicOzoneClientAdapterImpl.java @@ -204,7 +204,7 @@ public BasicOzoneClientAdapterImpl(String omHost, int omPort, OzoneClientUtils.resolveLinkBucketLayout(bucket, objectStore, new HashSet<>()); OzoneFSUtils.validateBucketLayout(bucket.getName(), resolvedBucketLayout); - } catch (IOException | IllegalArgumentException exception) { + } catch (IOException | RuntimeException exception) { // in case of exception, the adapter object will not be // initialised making the client object unreachable, close the client // to release resources in this case and rethrow. diff --git a/hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicRootedOzoneClientAdapterImpl.java b/hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicRootedOzoneClientAdapterImpl.java index 283dead9b0b..fefc87184ff 100644 --- a/hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicRootedOzoneClientAdapterImpl.java +++ b/hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicRootedOzoneClientAdapterImpl.java @@ -223,12 +223,12 @@ public BasicRootedOzoneClientAdapterImpl(String omHost, int omPort, // Fetches the bucket layout to be used by OFS. try { initDefaultFsBucketLayout(conf); - } catch (OMException ome) { + } catch (IOException | RuntimeException exception) { // in case of exception, the adapter object will not be // initialised making the client object unreachable, close the client // to release resources in this case and rethrow. ozoneClient.close(); - throw ome; + throw exception; } config = conf;