adoroszlai commented on code in PR #7285:
URL: https://github.com/apache/ozone/pull/7285#discussion_r1792369564
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/client/rpc/OzoneRpcClientTests.java:
##########
@@ -3140,6 +3149,34 @@ void testMultipartUploadOverride(ReplicationConfig
replication)
doMultipartUpload(bucket, keyName, (byte)97, replication);
}
+
+ @Test
+ public void testClientLeakDetector() throws Exception {
+ HddsUtils.setIgnoreReportingLeak(true);
+ OzoneClient client = OzoneClientFactory.getRpcClient(cluster.getConf());
+ String volumeName = UUID.randomUUID().toString();
+ String bucketName = UUID.randomUUID().toString();
+ String keyName = UUID.randomUUID().toString();
+ GenericTestUtils.LogCapturer ozoneClientFactoryLogCapturer =
+ GenericTestUtils.LogCapturer.captureLogs(
+ OzoneClientFactory.getLogger());
+
+ client.getObjectStore().createVolume(volumeName);
+ OzoneVolume volume = client.getObjectStore().getVolume(volumeName);
+ volume.createBucket(bucketName);
+ OzoneBucket bucket = volume.getBucket(bucketName);
+ byte[] data = new byte[10];
+ Arrays.fill(data, (byte) 1);
+ try (OzoneOutputStream out = bucket.createKey(keyName, 10,
+ ReplicationConfig.fromTypeAndFactor(RATIS, ONE), new HashMap<>())) {
+ out.write(data);
+ }
+ client = null;
+ System.gc();
+ GenericTestUtils.waitFor(() -> ozoneClientFactoryLogCapturer.getOutput()
+ .contains("is not closed correctly"), 100, 2000);
+ HddsUtils.setIgnoreReportingLeak(false);
Review Comment:
Calling `ozoneClientFactoryLogCapturer.clearOutput()` would also avoid
failure in post-processing due to this "leak". Wouldn't that be better than
adding a flag and special output just for the test?
Also, probably should be called in `finally` block.
##########
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}' \
+ | awk -F/ '{sub("-output.txt",""); print $NF ": Memory Leak detected in
test, Failing."}' \
Review Comment:
Let's not add this. Output is expected to contain test class names, extra
description will break processing below.
##########
hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicRootedOzoneClientAdapterImpl.java:
##########
@@ -221,7 +221,15 @@ public BasicRootedOzoneClientAdapterImpl(String omHost,
int omPort,
OzoneConfigKeys.HDDS_CONTAINER_IPC_PORT_DEFAULT);
// Fetches the bucket layout to be used by OFS.
- initDefaultFsBucketLayout(conf);
+ try {
+ initDefaultFsBucketLayout(conf);
+ } catch (OMException ome) {
Review Comment:
Shouldn't we catch `RuntimeException`, too?
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/AbstractOzoneFileSystemTest.java:
##########
@@ -2230,7 +2236,17 @@ void testFileSystemWithObjectStoreLayout() throws
IOException {
OzoneConfiguration config = new OzoneConfiguration(fs.getConf());
config.set(FS_DEFAULT_NAME_KEY, obsRootPath);
- IllegalArgumentException e =
assertThrows(IllegalArgumentException.class, () -> FileSystem.get(config));
+ IllegalArgumentException e =
+ assertThrows(IllegalArgumentException.class, () -> {
+ FileSystem fileSystem = null;
+ try {
+ fileSystem = FileSystem.get(config);
+ } finally {
+ if (fileSystem != null) {
+ fileSystem.close();
+ }
+ }
Review Comment:
Let's add a utility method (e.g. in `GenericTestUtils`) to close the object
if expected exception is not thrown.
```java
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) {
}
});
}
```
This let's us keep existing assertion statements (except we need to qualify
the call with `GenericTestUtils`:
```java
IllegalArgumentException e =
GenericTestUtils.assertThrows(IllegalArgumentException.class,
() -> FileSystem.get(config));
```
We can also reuse it e.g. for assertions about I/O:
https://github.com/apache/ozone/blob/d1e6900b957348ce50bc13cc5e30df3e1b7605fe/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/AbstractOzoneFileSystemTest.java#L412-L414
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/shell/TestOzoneShellHA.java:
##########
@@ -1149,8 +1148,6 @@ public void testListBucket() throws Exception {
getClientConfForOFS(hostPrefix, cluster.getConf());
int pageSize = 20;
clientConf.setInt(OZONE_FS_LISTING_PAGE_SIZE, pageSize);
- URI uri = FileSystem.getDefaultUri(clientConf);
- clientConf.setBoolean(String.format("fs.%s.impl.disable.cache",
uri.getScheme()), true);
Review Comment:
Is this related?
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/client/rpc/OzoneRpcClientTests.java:
##########
@@ -260,9 +265,12 @@ static void startCluster(OzoneConfiguration conf,
MiniOzoneCluster.Builder build
* Close OzoneClient and shutdown MiniOzoneCluster.
*/
static void shutdownCluster() throws IOException {
- if (ozClient != null) {
- ozClient.close();
+ for (OzoneClient ozoneClient : ozoneClients) {
+ if (ozoneClient != null) {
+ ozoneClient.close();
+ }
}
Review Comment:
```suggestion
org.apache.hadoop.hdds.utils.IOUtils.closeQuietly(ozoneClients);
```
##########
hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicOzoneClientAdapterImpl.java:
##########
@@ -193,18 +193,24 @@ public BasicOzoneClientAdapterImpl(String omHost, int
omPort,
OzoneClientFactory.getRpcClient(conf);
}
objectStore = ozoneClient.getObjectStore();
- this.volume = objectStore.getVolume(volumeStr);
- this.bucket = volume.getBucket(bucketStr);
- bucketReplicationConfig = this.bucket.getReplicationConfig();
- nextReplicationConfigRefreshTime =
- clock.millis() + bucketRepConfigRefreshPeriodMS;
-
- // resolve the bucket layout in case of Link Bucket
- BucketLayout resolvedBucketLayout =
- OzoneClientUtils.resolveLinkBucketLayout(bucket, objectStore,
- new HashSet<>());
-
- OzoneFSUtils.validateBucketLayout(bucket.getName(), resolvedBucketLayout);
+ try {
+ this.volume = objectStore.getVolume(volumeStr);
+ this.bucket = volume.getBucket(bucketStr);
+ bucketReplicationConfig = this.bucket.getReplicationConfig();
+ nextReplicationConfigRefreshTime = clock.millis() +
bucketRepConfigRefreshPeriodMS;
+
+ // resolve the bucket layout in case of Link Bucket
+ BucketLayout resolvedBucketLayout =
+ OzoneClientUtils.resolveLinkBucketLayout(bucket, objectStore, new
HashSet<>());
+
+ OzoneFSUtils.validateBucketLayout(bucket.getName(),
resolvedBucketLayout);
+ } catch (IOException | IllegalArgumentException exception) {
Review Comment:
Shouldn't we catch all `RuntimeException`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
