smengcl commented on code in PR #10989:
URL: https://github.com/apache/ozone/pull/10989#discussion_r4340244697
##########
hadoop-ozone/mini-cluster/src/main/java/org/apache/hadoop/ozone/MiniOzoneClusterProvider.java:
##########
@@ -215,14 +221,27 @@ private Thread createClusters() {
cluster = builder.build();
cluster.waitForClusterToBeReady();
createdCount++;
- clusters.put(cluster);
+ if (!addClusterResult(ClusterCreationResult.success(cluster))) {
+ cluster.shutdown();
+ break;
+ }
} catch (InterruptedException e) {
if (cluster != null) {
cluster.shutdown();
}
break;
} catch (IOException | TimeoutException e) {
- throw new RuntimeException("Unable to build cluster", e);
+ LOG.warn("Unable to build cluster", e);
Review Comment:
nit: when `build()` fails, `MiniOzoneClusterImpl` already logs "Unable to
build MiniOzoneCluster" with the stack trace, and `provide()` rethrows the
failure with this cause, so the trace gets reported twice. Consider logging
without the stack trace here.
```suggestion
LOG.warn("Unable to build cluster: {}", e.toString());
```
##########
hadoop-ozone/mini-cluster/src/main/java/org/apache/hadoop/ozone/MiniOzoneClusterProvider.java:
##########
@@ -231,10 +250,21 @@ private Thread createClusters() {
return t;
}
+ private boolean addClusterResult(ClusterCreationResult result)
+ throws InterruptedException {
Review Comment:
nit: fits within 120 chars, no need to wrap. Same for the
`ClusterCreationResult` constructor and the `new ClusterCreationResult()` in
`failure()`.
```suggestion
private boolean addClusterResult(ClusterCreationResult result) throws
InterruptedException {
```
##########
hadoop-ozone/mini-cluster/src/main/java/org/apache/hadoop/ozone/MiniOzoneClusterProvider.java:
##########
@@ -215,14 +221,27 @@ private Thread createClusters() {
cluster = builder.build();
cluster.waitForClusterToBeReady();
createdCount++;
- clusters.put(cluster);
+ if (!addClusterResult(ClusterCreationResult.success(cluster))) {
+ cluster.shutdown();
+ break;
+ }
} catch (InterruptedException e) {
if (cluster != null) {
cluster.shutdown();
}
break;
} catch (IOException | TimeoutException e) {
Review Comment:
`MiniOzoneClusterImpl.Builder.build()` rethrows `RuntimeException` as is
(e.g. a metrics "source already exists" error), so that still kills the create
thread and every later `provide()` waits 100s and fails with "Failed to obtain
available cluster in time", which is the cascade this PR fixes. `TestBuilder`
throws `IllegalStateException`. With the change below the test passes:
```suggestion
} catch (IOException | TimeoutException | RuntimeException e) {
```
Could you also add a test for it? For example:
```diff
if (result instanceof IOException) {
throw (IOException) result;
}
+ if (result instanceof RuntimeException) {
+ throw (RuntimeException) result;
+ }
```
```java
@Test
void retriesAfterRuntimeBuildFailure() throws Exception {
RuntimeException failure = new IllegalStateException("failed");
MiniOzoneCluster first = mock(MiniOzoneCluster.class);
MiniOzoneClusterProvider provider = new MiniOzoneClusterProvider(new
TestBuilder(failure, first), 1);
try {
IOException exception = assertThrows(IOException.class,
provider::provide);
assertSame(failure, exception.getCause());
assertSame(first, provider.provide());
} finally {
provider.shutdown();
}
}
```
##########
hadoop-ozone/mini-cluster/src/test/java/org/apache/hadoop/ozone/TestMiniOzoneClusterProvider.java:
##########
@@ -0,0 +1,135 @@
+/*
+ * 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.hadoop.ozone;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertSame;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTimeoutPreemptively;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.mockito.Mockito.doAnswer;
+import static org.mockito.Mockito.doThrow;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.verify;
+
+import java.io.IOException;
+import java.time.Duration;
+import java.util.ArrayDeque;
+import java.util.Arrays;
+import java.util.Queue;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.TimeoutException;
+import java.util.concurrent.atomic.AtomicInteger;
+import org.apache.hadoop.hdds.conf.OzoneConfiguration;
+import org.junit.jupiter.api.Test;
+
+class TestMiniOzoneClusterProvider {
+
+ @Test
+ void retriesAfterBuildFailure() throws Exception {
+ IOException failure = new IOException("failed");
+ MiniOzoneCluster first = mock(MiniOzoneCluster.class);
+ MiniOzoneCluster second = mock(MiniOzoneCluster.class);
+ TestBuilder builder = new TestBuilder(failure, first, second);
+ MiniOzoneClusterProvider provider = new MiniOzoneClusterProvider(builder,
+ 2);
Review Comment:
nit: fits within 120 chars, no need to wrap. Same for the other `new
MiniOzoneClusterProvider()` and `assertThrows(IOException.class,
provider::provide)` calls, and the
`shutdownDoesNotBlockWhenClusterCreationConsumesInterrupt() throws Exception`
signature.
```suggestion
MiniOzoneClusterProvider provider = new
MiniOzoneClusterProvider(builder, 2);
```
--
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]
