adoroszlai commented on a change in pull request #1806:
URL: https://github.com/apache/ozone/pull/1806#discussion_r568552958
##########
File path:
hadoop-hdds/config/src/test/java/org/apache/hadoop/hdds/conf/TestConfigFileGenerator.java
##########
@@ -36,7 +36,8 @@
@Test
public void testGeneratedXml() throws FileNotFoundException {
String generatedXml =
- new Scanner(new
File("target/test-classes/ozone-default-generated.xml"))
+ new Scanner(new
File("target/test-classes/ozone-default-generated.xml"),
+ "UTF-8")
Review comment:
Please replace `"UTF-8"` with `StandardCharsets.UTF_8.name()`.
Similarly in few other locations where this string is being added. This was
recently improved in #1673, we should try to avoid regression.
##########
File path:
hadoop-ozone/fault-injection-test/mini-chaos-tests/src/test/java/org/apache/hadoop/ozone/failure/FailureManager.java
##########
@@ -68,7 +68,7 @@ private void fail() {
f.fail(cluster);
} catch (Throwable t) {
LOG.info("Caught exception while inducing failure:{}", f.getName(), t);
- System.exit(-2);
+ throw new RuntimeException();
Review comment:
@mukul1987 will this keep existing behavior of chaos tests?
##########
File path:
hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/server/TestSCMBlockProtocolServer.java
##########
@@ -50,7 +50,7 @@
private StorageContainerManager scm;
private NodeManager nodeManager;
private ScmBlockLocationProtocolServerSideTranslatorPB service;
- private final int nodeCount = 10;
+ private static final int NODECOUNT = 10;
Review comment:
`NODE_COUNT`
##########
File path:
hadoop-ozone/recon/src/test/java/org/apache/hadoop/ozone/recon/api/TestEndpoints.java
##########
@@ -133,27 +131,24 @@
private Pipeline pipeline;
private FileCountBySizeDao fileCountBySizeDao;
private DSLContext dslContext;
- private final String host1 = "host1.datanode";
- private final String host2 = "host2.datanode";
- private final String ip1 = "1.1.1.1";
- private final String ip2 = "2.2.2.2";
- private final String prometheusTestResponseFile =
+ private static final String HOST1 = "host1.datanode";
+ private static final String HOST2 = "host2.datanode";
+ private static final String IP1 = "1.1.1.1";
+ private static final String IP2 = "2.2.2.2";
+ private static final String PROMETHEUSTESTRESPONSEFILE =
Review comment:
`PROMETHEUS_TEST_RESPONSE_FILE`
##########
File path:
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/contract/AbstractContractUnbufferTest.java
##########
@@ -84,7 +84,9 @@ public void testUnbufferOnClosedFile() throws IOException {
FSDataInputStream stream = null;
try {
stream = getFileSystem().open(file);
- validateFullFileContents(stream);
+ if (stream != null) {
+ validateFullFileContents(stream);
Review comment:
Instead of the null check, can we add an `assertNotNull` in
`validateFileContents`?
##########
File path:
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/security/TestOMGetDelegationTokenRequest.java
##########
@@ -56,7 +56,7 @@
private OMRequest originalRequest;
private OMRequest modifiedRequest;
private OMGetDelegationTokenRequest omGetDelegationTokenRequest;
- private final String checkResponse = "";
+ private static final String CHECKRESPONSE = "";
Review comment:
`CHECK_RESPONSE`
##########
File path:
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMKeyRequest.java
##########
@@ -78,8 +79,8 @@
protected OzoneBlockTokenSecretManager ozoneBlockTokenSecretManager;
protected ScmBlockLocationProtocol scmBlockLocationProtocol;
- protected final long containerID = 1000L;
- protected final long localID = 100L;
+ protected static final long CONTAINERID = 1000L;
+ protected static final long LOCALID = 100L;
Review comment:
`CONTAINER_ID` and `LOCAL_ID` would be more in line with naming
conventions.
##########
File path:
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/client/rpc/TestOzoneClientRetriesOnException.java
##########
@@ -62,7 +62,7 @@
/**
* Tests failure detection and handling in BlockOutputStream Class.
*/
-public class TestOzoneClientRetriesOnException {
+public class TestOzoneClientRetriesOnException extends Exception {
Review comment:
Seems to be a false positive. `TestOzoneClientRetriesOnException` means
this class tests that the client does retry when it encounters exceptions. I
think it could be renamed to `TestOzoneClientRetriesOnExceptions` to satisfy
spotbugs.
##########
File path:
hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/placement/algorithms/TestContainerPlacementFactory.java
##########
@@ -55,7 +55,7 @@
// datanodes array list
private List<DatanodeDetails> datanodes = new ArrayList<>();
// node storage capacity
- private final long storageCapacity = 100L;
+ private static final long STORAGECAPACITY = 100L;
Review comment:
`STORAGE_CAPACITY`
##########
File path:
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/MiniOzoneHAClusterImpl.java
##########
@@ -250,7 +250,7 @@ public void stopOzoneManager(String omNodeId) {
*/
public static class Builder extends MiniOzoneClusterImpl.Builder {
- private final String nodeIdBaseStr = "omNode-";
+ private static final String NODEIDBASESTR = "omNode-";
Review comment:
How about `NODE_ID_PREFIX`?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
