[
https://issues.apache.org/jira/browse/HADOOP-19970?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18133873#comment-18133873
]
ASF GitHub Bot commented on HADOOP-19970:
-----------------------------------------
joseluisll commented on PR #8699:
URL: https://github.com/apache/hadoop/pull/8699#issuecomment-6483615149
Thanks @pjfanning and @pan3793 for the detailed reviews. This PR had grown
into several unrelated changes, so I have split it up and rescoped
HADOOP-19970. Here is the plan, followed by where each review comment ended up.
### How the work is split
Everything is now a sub-task of HADOOP-19912 (Jetty 12 on ee8). Each one is
a single commit that applies cleanly on current trunk, makes sense on its own,
and can be reviewed and reverted on its own:
| JIRA | Scope |
|---|---|
| HADOOP-20012 | Exclude Solr's Jetty 9.4.44 and the extra servlet API from
the catalog-webapp and timelineservice-hbase tests |
| HADOOP-20013 | Remove the duplicate javax.annotation-api
(jakarta.annotation-api already provides the classes), with a test |
| HADOOP-20014 | Run the Jersey tests on the JDK HTTP server instead of
Jetty |
| HADOOP-20015 | Declare the Jetty web artifacts that yarn-common, the RM,
the AHS and the Router use directly |
| HADOOP-20016 | Declare jetty-http (optional) in the modules that import it
|
| **HADOOP-19970** (rescoped) | Move the servlet API to
org.eclipse.jetty.toolchain:jetty-servlet-api 4.0.9, plus the classpath tests
(one servlet API, one Jetty release) |
The JobEndNotifier logging change went into its own PR, as @pan3793
suggested: MAPREDUCE-7544 (#8751), now merged.
Except for HADOOP-19970, none of these changes Jetty, the servlet API or
anything downstream projects resolve. They only make dependencies explicit, or
remove duplicates and third-party Jetty copies from test classpaths. I have
also checked the six together: with all of them applied, every module resolves
one servlet API (jetty-servlet-api 4.0.9) and one Jetty release (9.4.58).
### Review comments
Every comment has been taken into account:
| Comment | Where | Outcome |
|---|---|---|
| Six near-identical TestJettyClasspath copies | HADOOP-19970 | One
AbstractJettyClasspathTest in the hadoop-common test jar; each module's
TestJettyClasspath is an empty subclass |
| Regex only allowed an optional websocket/ segment | HADOOP-19970 | The
path regex is gone (next row); any jar with classes under org/eclipse/jetty/
counts, whatever its group (http2, alpn, fcgi, websocket, ...) |
| Detection depended on the Maven repository layout | HADOOP-19970 | Jars
are recognised by their contents and manifests: a Jetty jar has classes under
org/eclipse/jetty/ and its release is the manifest's Implementation-Version.
The servlet API provider is checked by Bundle-SymbolicName. Checked in a Maven
layout, a Gradle cache layout and a flattened lib directory: the tests pass
with one release and fail when a stale Jetty jar or a second servlet API is
added |
| Solr exclusions repeated on both Solr dependencies | HADOOP-20012 |
solr-test-framework already brings solr-core, so solr-core is no longer
declared and the exclusions live once, on solr-test-framework. I kept them in
the module rather than in hadoop-project because only that module uses Solr.
The resolved test classpath is unchanged |
| jetty-util back in hadoop-client-runtime | HADOOP-20014 | Done the
narrower way you suggested: hadoop-client-runtime is untouched (YARN-11793's
excludes stay, so 0 jetty-util / jetty-continuation classes), and
hadoop-client-minicluster ships jetty-util itself |
| jetty-servlet-api changes things for downstream projects | HADOOP-19970 |
There is now a release note, and a comment on the managed entry in
hadoop-project: projects that also declare javax.servlet-api or
jakarta.servlet-api 4.x should exclude one of them. Why not keep
javax.servlet-api: jetty-servlet-api 4.0.9 is the artifact and version Jetty
12's ee8 environment manages, so downstream projects get the coordinate change
once, here, not again with Jetty 12 |
| jetty-http in the shuffle handler should be provided | HADOOP-20016 | It
is now optional, so it is not passed on to consumers and is not copied into the
mapreduce lib directory. I kept optional rather than provided because on trunk
the shuffle handler already resolves jetty-http at compile scope (through the
NodeManager and mapreduce-client-common). provided would change that
resolution, while optional keeps it as it is |
| Dead org.eclipse.jetty:javax.servlet-api exclusion kept beside the new one
| HADOOP-19970 | Replaced, not kept beside |
| jakarta.servlet-api excluded on -core, the provider and in minicluster |
HADOOP-19970 | Only the managed exclusion on jersey-test-framework-core
remains, with a note. Managed exclusions also apply when the provider pulls in
core transitively, and to minicluster's own declaration of core. The test
classpaths are identical with and without the other exclusions; without the
managed one, jakarta.servlet-api comes back in 17 modules |
| (@pan3793) Jetty Log in JobEndNotifier, SLF4J with {} placeholders |
MAPREDUCE-7544 | Merged in #8751 |
| (@pan3793) why provider-grizzly2, would provider-simple do? | HADOOP-20014
| Neither: the tests now use jersey-test-framework-provider-jdk-http, which
runs on the JDK's built-in HTTP server and needs no third-party server |
| (@pan3793) javax.servlet vs jakarta.servlet during the namespace migration
| HADOOP-19970 | With javax.servlet coming from jetty-servlet-api, the
jakarta.servlet:jakarta.servlet-api coordinate is free for the 5.x/6.x
jakarta.* API, so both namespaces can sit side by side during the migration
without two versions of the same artifact |
### Question: one ticket or several?
Before I update this PR and open the others: do you prefer to review this as
**one single ticket / PR** (HADOOP-19970 with everything above, as before) or
as **several split-up tickets**, one PR per sub-task above?
My suggestion is the split. The PRs are small and independent of each other:
the only overlap is one line in NOTICE-binary between HADOOP-19970 and
HADOOP-20013. The split also lets the five changes that do not alter what
downstream projects resolve go in first, while the servlet API move gets its
own discussion. If you prefer one PR, I will fold them back into this one,
keeping one commit per sub-task so it can still be reviewed piece by piece.
> HADOOP-Move the servlet API to jetty-servlet-api 4.0.9
> ------------------------------------------------------
>
> Key: HADOOP-19970
> URL: https://issues.apache.org/jira/browse/HADOOP-19970
> Project: Hadoop Common
> Issue Type: Sub-task
> Components: build, hadoop-common
> Reporter: Jose Luis López
> Assignee: Jose Luis López
> Priority: Minor
> Labels: pull-request-available
>
> *Scope*:
> * hadoop-project/pom.xml: add the jetty-servlet-api.version property (4.0.9),
> and replace the managed jakarta.servlet:jakarta.servlet-api 4.0.4 with
> org.eclipse.jetty.toolchain:jetty-servlet-api.
> * hadoop-common/pom.xml: declare
> org.eclipse.jetty.toolchain:jetty-servlet-api at compile scope. HttpServer2
> and its filters compile against javax.servlet, which today arrives only as
> javax.servlet-api 3.1.0 through jetty-server. Ten modules that use
> javax.servlet without declaring an API get it through hadoop-common.
> * hadoop-project/pom.xml and hadoop-tools/hadoop-sls/pom.xml: the exclusion
> on jetty-server names org.eclipse.jetty:javax.servlet-api, a coordinate that
> matches nothing. Change it to javax.servlet:javax.servlet-api.
> * hadoop-project/pom.xml: on javax-websocket-server-impl, also exclude
> javax.servlet:javax.servlet-api (through websocket-servlet).
> * Switch the coordinate in the 17 module poms that declare the servlet API.
> * Rename the 5 jakarta.servlet-api exclusions: 4 in hadoop-client-minicluster
> and 1 in hadoop-yarn-common.
> * jersey-test-framework-core declares jakarta.servlet-api at compile scope.
> Exclude it there, and change the exclusion on the jdk-http provider to the
> same coordinate.
> * LICENSE-binary: replace jakarta.servlet:jakarta.servlet-api:4.0.4 with
> org.eclipse.jetty.toolchain:jetty-servlet-api:4.0.9.
> * LICENSE-binary and NOTICE-binary: drop javax.servlet:javax.servlet-api
> 3.1.0.
> *Why*: javax.servlet-api 3.1.0 and jakarta.servlet-api 4.0.4 both publish the
> javax.servlet packages, and which one a module compiles and runs against
> depends on jar order. jetty-servlet-api is the servlet API that Jetty 12's
> ee8 environment depends on. Switching now leaves one servlet API jar and
> takes the coordinate change out of HADOOP-19972.
> *Impact on Jetty 9*: none. 4.0 is a superset of 3.1, and Jetty 9.4 runs on
> Servlet 4.0. jetty-servlet-api and jakarta.servlet-api 4.0.4 ship the same
> javax.servlet 4.0 classes, and jetty-servlet-api also carries the web.xml
> schemas. The project's web.xml files declare versions 2.4, 2.5 and 3.0, which
> Servlet 4.0 accepts.
> *Downstream*:
> * hadoop-client-runtime: still relocates one servlet API under
> org.apache.hadoop.shaded.javax.servlet. Its source changes from
> jakarta.servlet-api 4.0.4 to jetty-servlet-api 4.0.9: same javax.servlet 4.0
> classes, plus the web.xml schemas. No public API change.
> * hadoop-client-api, hadoop-client-minicluster: unchanged; both already
> exclude the servlet API.
> * Non-shaded artifacts: the coordinate changes from
> jakarta.servlet:jakarta.servlet-api to
> org.eclipse.jetty.toolchain:jetty-servlet-api. Downstream exclusions of
> jakarta.servlet-api need the new coordinate.
> * Direct consumers of hadoop-common: the servlet API changes from
> javax.servlet:javax.servlet-api 3.1.0 to jetty-servlet-api 4.0.9. Same
> javax.servlet packages; 4.0 is a superset of 3.1.
> *Depends on*: HADOOP-20014 (exclusion on the jdk-http provider).
> *CI*: touches hadoop-project, so a full build.
--
This message was sent by Atlassian Jira
(v10.3.26#10030026)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
