Skip to content

Commit

Permalink
address comments
Browse files Browse the repository at this point in the history
  • Loading branch information
Sadanand Shenoy committed Oct 9, 2024
1 parent d1e6900 commit d1389e6
Show file tree
Hide file tree
Showing 7 changed files with 33 additions and 43 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -113,8 +113,6 @@ public final class HddsUtils {

private static final int NO_PORT = -1;

private static boolean ignoreReportingLeak = false;

private HddsUtils() {
}

Expand Down Expand Up @@ -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());
Expand All @@ -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);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -205,6 +206,20 @@ public static void waitFor(BooleanSupplier check, int checkEveryMillis,
}
}

public static <T extends Throwable> T assertThrows(
Class<T> expectedType,
Callable<? extends AutoCloseable> func) {
return Assertions.assertThrows(expectedType, () -> {
final AutoCloseable closeable = func.call();
try {
if (closeable != null) {
closeable.close();
}
} catch (Exception ignored) {
}
});
}

/**
* @deprecated use sl4fj based version
*/
Expand Down
2 changes: 1 addition & 1 deletion hadoop-ozone/dev-support/checks/_mvn_unit_report.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Expand Down Expand Up @@ -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");
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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();
Expand All @@ -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 {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down

0 comments on commit d1389e6

Please sign in to comment.