adoroszlai commented on code in PR #10478:
URL: https://github.com/apache/ozone/pull/10478#discussion_r3405343635
##########
hadoop-ozone/client/src/test/java/org/apache/hadoop/ozone/client/rpc/TestRpcClient.java:
##########
@@ -18,14 +18,26 @@
package org.apache.hadoop.ozone.client.rpc;
import static org.apache.hadoop.ozone.client.rpc.RpcClient.validateOmVersion;
+import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertThrows;
+import java.io.IOException;
import java.util.LinkedList;
import java.util.List;
+import org.apache.hadoop.hdds.conf.OzoneConfiguration;
import org.apache.hadoop.hdds.protocol.proto.HddsProtos;
+import org.apache.hadoop.hdds.scm.XceiverClientFactory;
+import org.apache.hadoop.ozone.client.MockOmTransport;
+import org.apache.hadoop.ozone.client.MockXceiverClientFactory;
import org.apache.hadoop.ozone.OzoneManagerVersion;
import org.apache.hadoop.ozone.om.helpers.ServiceInfo;
+import org.apache.hadoop.ozone.om.helpers.ServiceInfoEx;
+import org.apache.hadoop.ozone.om.protocolPB.OmTransport;
+import org.apache.log4j.Logger;
+import org.apache.log4j.PatternLayout;
+import org.apache.ozone.test.GenericTestUtils.LogCapturer;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.EnumSource;
Review Comment:
Imports needed for logger suggestion:
```suggestion
import org.junit.jupiter.params.provider.EnumSource;
import org.slf4j.event.Level;
```
##########
hadoop-ozone/client/src/test/java/org/apache/hadoop/ozone/client/rpc/TestRpcClient.java:
##########
@@ -215,4 +227,41 @@ public void
testFutureVersionShouldNotBeAnExpectedVersion() {
IllegalArgumentException.class,
() -> validateOmVersion(OzoneManagerVersion.FUTURE_VERSION, null));
}
+
+ @Test
+ public void testCloseTwiceDoesNotWarn() throws IOException {
+ RpcClient rpcClient = createRpcClient();
+ LogCapturer logs = LogCapturer.captureLogs(Logger.getRootLogger(),
+ new PatternLayout("%p %c %m%n"));
Review Comment:
- `IOUtils.cleanupWithLogger` logs exceptions at `DEBUG`, so we need to set
at least that level if we want to make sure it does not encounter any.
Otherwise the assertion about log content may succeed even without the fix.
- `PatternLayout` and `getRootLogger` are unnecessary.
```suggestion
GenericTestUtils.setLogLevel(RpcClient.class, Level.DEBUG);
LogCapturer logs = LogCapturer.captureLogs(RpcClient.class);
```
##########
hadoop-ozone/client/src/test/java/org/apache/hadoop/ozone/client/rpc/TestRpcClient.java:
##########
@@ -18,14 +18,26 @@
package org.apache.hadoop.ozone.client.rpc;
import static org.apache.hadoop.ozone.client.rpc.RpcClient.validateOmVersion;
+import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertThrows;
+import java.io.IOException;
import java.util.LinkedList;
import java.util.List;
+import org.apache.hadoop.hdds.conf.OzoneConfiguration;
import org.apache.hadoop.hdds.protocol.proto.HddsProtos;
+import org.apache.hadoop.hdds.scm.XceiverClientFactory;
+import org.apache.hadoop.ozone.client.MockOmTransport;
+import org.apache.hadoop.ozone.client.MockXceiverClientFactory;
import org.apache.hadoop.ozone.OzoneManagerVersion;
import org.apache.hadoop.ozone.om.helpers.ServiceInfo;
+import org.apache.hadoop.ozone.om.helpers.ServiceInfoEx;
+import org.apache.hadoop.ozone.om.protocolPB.OmTransport;
+import org.apache.log4j.Logger;
+import org.apache.log4j.PatternLayout;
Review Comment:
Imports needed for logger suggestion:
```suggestion
import org.apache.ozone.test.GenericTestUtils;
```
##########
hadoop-ozone/client/src/test/java/org/apache/hadoop/ozone/client/rpc/TestRpcClient.java:
##########
@@ -18,14 +18,26 @@
package org.apache.hadoop.ozone.client.rpc;
import static org.apache.hadoop.ozone.client.rpc.RpcClient.validateOmVersion;
+import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
Review Comment:
Imports for `assertThat` suggestion:
```suggestion
import static org.assertj.core.api.Assertions.assertThat;
import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
import static org.junit.jupiter.api.Assertions.assertEquals;
```
##########
hadoop-ozone/client/src/test/java/org/apache/hadoop/ozone/client/rpc/TestRpcClient.java:
##########
@@ -215,4 +227,41 @@ public void
testFutureVersionShouldNotBeAnExpectedVersion() {
IllegalArgumentException.class,
() -> validateOmVersion(OzoneManagerVersion.FUTURE_VERSION, null));
}
+
+ @Test
+ public void testCloseTwiceDoesNotWarn() throws IOException {
+ RpcClient rpcClient = createRpcClient();
+ LogCapturer logs = LogCapturer.captureLogs(Logger.getRootLogger(),
+ new PatternLayout("%p %c %m%n"));
+ logs.clearOutput();
+
+ try {
+ assertDoesNotThrow(() -> {
+ rpcClient.close();
+ rpcClient.close();
+ });
+
+ String output = logs.getOutput();
+ assertFalse(output.contains("WARN"), output);
+ assertFalse(output.contains("This metrics class is not used."), output);
Review Comment:
Please use `assertThat` instead of `assertTrue`/`assertFalse` for types
where it provides assertions with more useful failure messages.
```suggestion
assertThat(logs.getOutput())
.doesNotContain("WARN")
.doesNotContain("This metrics class is not used.");
```
Compare:
```
expected: <false> but was: <true>
```
to
```
Expecting actual:
"... [ForkJoinPool-1-worker-1] DEBUG rpc.RpcClient
(IOUtils.java:cleanupWithLogger(61)) - Exception in closing
org.apache.hadoop.hdds.scm.ContainerClientMetrics$Handle@4fd1e151
java.lang.IllegalStateException: This metrics class is not used.
...
"
not to contain:
"This metrics class is not used."
```
--
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]
