[
https://issues.apache.org/jira/browse/HADOOP-20015?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18138656#comment-18138656
]
ASF GitHub Bot commented on HADOOP-20015:
-----------------------------------------
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);
```
> Stop using Jetty 9.4 APIs that Jetty 12 removes
> -----------------------------------------------
>
> Key: HADOOP-20015
> URL: https://issues.apache.org/jira/browse/HADOOP-20015
> Project: Hadoop Common
> Issue Type: Sub-task
> Reporter: Jose Luis López
> Assignee: Jose Luis López
> Priority: Major
> Labels: pull-request-available
>
> *Scope*: every use, in main and test code across the repository, of a Jetty
> 9.4 class that Jetty 12 no longer has, where the JDK, SLF4J or the servlet
> API does the same job:
> * FSMainOperationsBaseTest, ViewFileSystemTestSetup, ViewFsTestSetup,
> TestAuthenticationSessionCookie (hadoop-common),
> TestRollingLevelDBTimelineStore (applicationhistoryservice), TestAMRMClient,
> TestYarnCLI (yarn-client) and MockNM (resourcemanager): they logged through
> org.eclipse.jetty.util.log.Log, which Jetty 10 and later no longer have. Log
> through SLF4J instead.
> * TestDockerCommandExecutor (nodemanager): it borrowed a logger from Jetty's
> GzipHttpOutputInterceptor. Give it its own.
> * FederationInterceptor (nodemanager): replace Jetty's ConcurrentHashSet,
> which Jetty 12 no longer has, with ConcurrentHashMap.newKeySet().
> * TestLogAggregationService (nodemanager): it batched assertion failures in
> Jetty's MultiException, which Jetty 12 no longer has. Collect them in a list
> and throw one AssertionError carrying all of them.
> * HttpServer2#inferMimeType (hadoop-common): drop the cast to a Jetty
> context, since ServletContext has getMimeType.
> *Out of scope*: classes that Jetty 12 moves to the org.eclipse.jetty.ee8
> packages, and uses whose only replacement is another Jetty API (such as
> MultiException in HttpServer2). Those are the upgrade itself, HADOOP-19972.
> Every other Jetty class the code imports still exists in Jetty 12.
> *Why*: these uses would not compile on Jetty 12, and none of them needs
> Jetty. Removing them now keeps HADOOP-19972 to the changes the upgrade really
> requires.
> *Impact on Jetty 9*: none. Java changes only, no pom change; the logging,
> set, failure-batching and MIME-type code behaves the same.
> *Downstream*: none.
> *CI*: hadoop-common, applicationhistoryservice, yarn-client, resourcemanager,
> nodemanager.
> *PR*: https://github.com/apache/hadoop/pull/8893
> Regrouped on 2026-11-04 by kind of change, as suggested in the review: this
> issue now holds all the Java changes that were split between HADOOP-20015
> (YARN) and HADOOP-20016 (HADOOP); HADOOP-20016 holds all the pom changes.
--
This message was sent by Atlassian Jira
(v10.3.26#10030026)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
