will-sh commented on PR #6347:
URL: https://github.com/apache/ozone/pull/6347#issuecomment-1984948061
Regarding the remaining tasks, I'm not sure if my understanding is correct:
* Test cases can now use MiniOzoneCluster wrapped in try-with-resources
blocks to ensure the cluster is shut down even if waitForClusterToBeReady() and
other operations fail.
Before:
```
private void testRpcPermissionWithConf(
OzoneConfiguration ozoneConf,
Predicate<String> isAdmin,
String... usernames) throws Exception {
MiniOzoneCluster cluster =
MiniOzoneCluster.newBuilder(ozoneConf).build();
try {
cluster.waitForClusterToBeReady();
for (String username : usernames) {
testRpcPermission(cluster, username,
!isAdmin.test(username));
}
} finally {
cluster.shutdown();
}
}
```
After:
```
private void testRpcPermissionWithConf(
OzoneConfiguration ozoneConf,
Predicate<String> isAdmin,
String... usernames) throws Exception {
try (MiniOzoneCluster cluster =
MiniOzoneCluster.newBuilder(ozoneConf).build()) {
cluster.waitForClusterToBeReady();
for (String username : usernames) {
testRpcPermission(cluster, username,
!isAdmin.test(username));
}
} // The cluster is automatically closed here, no need to call
cluster.shutdown()
}
```
* Convert try-catch blocks to assertThrows() where applicable (where
exception is expected and try block calls fail().
I only found one place that might need to be refactored
Before:
```
try {
ContainerWithPipeline container2 = mockClientServer.allocateContainer(
HddsProtos.ReplicationType.RATIS,
HddsProtos.ReplicationFactor.ONE, OzoneConsts.OZONE);
if (expectPermissionDenied) {
fail("Operation should fail, expecting an IOException here.");
} else {
assertEquals(1, container2.getPipeline().getNodes().size());
}
} catch (Exception e) {
verifyPermissionDeniedException(e, fakeRemoteUsername);
}
```
After:
```
if (expectPermissionDenied) {
Exception ex = assertThrows(Exception.class, () -> {
ContainerWithPipeline container2 =
mockClientServer.allocateContainer(
HddsProtos.ReplicationType.RATIS,
HddsProtos.ReplicationFactor.ONE, OzoneConsts.OZONE);
});
verifyPermissionDeniedException(ex, fakeRemoteUsername);
} else {
ContainerWithPipeline container2 = mockClientServer.allocateContainer(
HddsProtos.ReplicationType.RATIS,
HddsProtos.ReplicationFactor.ONE, OzoneConsts.OZONE);
assertEquals(1, container2.getPipeline().getNodes().size());
}
```
Could you please advise?
--
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]
