pan3793 commented on code in PR #8893:
URL: https://github.com/apache/hadoop/pull/8893#discussion_r4479221324
##########
hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/FSMainOperationsBaseTest.java:
##########
@@ -58,6 +59,9 @@
* </p>
*/
public abstract class FSMainOperationsBaseTest extends FileSystemTestHelper {
+ private static final Logger LOG =
+ LoggerFactory.getLogger(FSMainOperationsBaseTest.class);
+
Review Comment:
nit: drop this blank line, since the next line is already blank. Same in
`ViewFileSystemTestSetup`, `ViewFsTestSetup`, `TestAMRMClient`,
`TestDockerCommandExecutor` and `MockNM`.
##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/http/HttpServer2.java:
##########
@@ -1960,9 +1960,7 @@ public void doFilter(ServletRequest request,
*/
private String inferMimeType(ServletRequest request) {
String path = ((HttpServletRequest)request).getRequestURI();
- ServletContextHandler.Context sContext =
- (ServletContextHandler.Context)config.getServletContext();
- String mime = sContext.getMimeType(path);
+ String mime = config.getServletContext().getMimeType(path);
return (mime == null) ? null : mime;
Review Comment:
nit: this ternary does nothing, so `inferMimeType` can return
`config.getServletContext().getMimeType(path)` directly.
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-client/src/test/java/org/apache/hadoop/yarn/client/api/impl/TestAMRMClient.java:
##########
@@ -87,14 +87,18 @@
import org.mockito.ArgumentCaptor;
import org.mockito.invocation.InvocationOnMock;
import org.mockito.stubbing.Answer;
-import org.eclipse.jetty.util.log.Log;
import static org.junit.jupiter.api.Assumptions.assumeTrue;
+import org.slf4j.Logger;
Review Comment:
nit: move the `org.slf4j` imports up with the other non-static imports. Same
in `TestAuthenticationSessionCookie` and `TestDockerCommandExecutor`.
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/test/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/logaggregation/TestLogAggregationService.java:
##########
@@ -1495,7 +1494,11 @@ void checkEvents(EventHandler<T> eventHandler,
}
}
}
- failures.ifExceptionThrow();
+ if (!failures.isEmpty()) {
+ AssertionError error = new AssertionError(failures.size() + " failures");
Review Comment:
`MultiException` rethrew a single failure as is, so the summary showed the
assertion message. This shows `1 failures`, with the detail only in the
suppressed trace. `MultipleFailuresError`, which is what `assertAll` throws,
includes every failure in its message. It needs `import
org.opentest4j.MultipleFailuresError;`, and opentest4j comes with
`junit-jupiter-api`.
```suggestion
MultipleFailuresError error = new MultipleFailuresError("checkEvents",
failures);
```
--
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]
