[MRESOLVER-308] HTTP transport showdown. #231

cstamas · 2022-12-22T15:24:56Z

Introduces 3 2 new transport: Jetty HttpClient 10.x, ~~OkHttp 4.x~~ and Java 11 HttpClient based ones. These are all HTTP/2 capable transports. 2 out of 3 new ones requires Java 11, hence corresponding modules lavel raised to Java11 and Java 11 is required to build (sniffer removed, using compiler release flag) but the library still remains Java 8!

Also, some reshuffle around HTTP tests, made them reusable to be able to use them in all HTTP based transports.

High level changes:

introduced transport-okhttp using OkHttp 4.10.x
introduced transport-jetty using Jetty Client 10.x, needs Java 11
introduced transport-java11 using Java 11 HttpClient, needs Java 11
build required minimal Java upped to Java 11, but project still produces Java 8 binaries (except for 2 new transports)
animal-sniffer removed, using compiler release=8 flag (except for 2 new transports) and enforcer byteCode rule adjusted to ignore test scoped dependencies.
HTTP test utilities (HTTP Server using EOL Jetty 9.x and other) moved out of transport-http into maven-resolver-test-http module, made it reusable and updated: it is now implemented using Jetty 10.x and is HTTP/2 capable (when TLS enabled). This implies that Java 8 modules uses Java 11 test libraries for testing (the HTTP server), hence the enforcer bytecode rule adjustment.
transport specific tests moved in their corresponding UT class that extends the common HTTP test support class. Hence all of HTTP transports run same superset of HTTP tests (from reusable HttpTransporterTestSupport).

Important notes

none of the new transports supports the "WebDAV MKCOL dance". This logic originates from Maven 2.x Wagon era, when people used httpd+mod_dav to implement hosted repositories. All new transports assume modern environments, use of proper Maven Repository Managers and not some "home fashioned solutions".
all the new transports supports only HTTP Basic authentication, that should be used in combination with HTTPS, just the usual.
tested with mvn only. In mvnd these will NOT properly work, as "onSessionClose event" is still missing: clients are created per session, but are NOT stopped when session ends -- hence it works in mvn as process exits on session end, but mvnd does not. All this is resolved with [MRESOLVER-302] Introduce onSessionClose hooks #232
and the final notice: not for production use (until it proves otherwise).

For perf tests (kinda) look at related JIRA issue https://issues.apache.org/jira/browse/MRESOLVER-308

New Transports

This PR introduces 3 new transports for resolver:

Java 11 HttpClient transport

Uses Java 11 HttpClient to implement transport, it prefers HTTP/2 and supports HTTP/1.1.

The build produces one 27KB JAR, that can be placed into Maven lib/ext of any Maven version that uses Resolver 1.9.2+. The transport remains dormant when Maven runs on Java 8, while activates (and becomes default) when Java 11+ used to run Maven.

Jetty 10.x Client transport

Uses Jetty 10.x Client to implement transport. When remote repository is HTTPS, it prefers HTTP/2 and supports HTTP/1.1, otherwise speaks HTTP/1.1 only.

The build produces "pgk.zip" classified artifact with transport and all of it's needed dependencies. To use it, unpack the ZIP into Maven lib/ext of any Maven version that uses Resolver 1.9.2+ and use Java 11 or above to run Maven.

OkHttp 4.10.x transport

~~Uses OkHttp 4.10.x to implement transport. Supports HTTP/2 and HTTP/1.1.~~

~~The build produces "pgk.zip" classified artifact with transport and all of it's needed dependencies. To use it, unpack the ZIP into Maven lib/ext of any Maven version that uses Resolver 1.9.2+.~~

OkHttp transport is about to be dropped.

Note:

tested with Maven Central (supports HTTP/2 and HTTP/1.1) https://repo.maven.apache.org/maven2/
tested with EU Google Mirror of Maven Central (supports HTTP/3, HTTP/2 and HTTP/1.1) https://maven-central-eu.storage-download.googleapis.com/maven2/

https://issues.apache.org/jira/browse/MRESOLVER-308

Introduces 3 new transport: Jetty HttpClient 10.x, OkHttp 4.x and Java 11 HttpClient based ones. These are all HTTP/2 capable transports. Also, some reshuffle around HTTP tests, made them reusable to be able to use them in all HTTP based transports. --- https://issues.apache.org/jira/browse/MRESOLVER-308

michael-o

This should bump the version to 1.10 due to the build requirement change.

kwin · 2022-12-24T00:19:38Z

Do we really want to keep maintaining that many resolver transport modules? If there is a clear winner performance wise, why not dropping the other modules? I fail to see the advantage of providing that many options...

michael-o · 2022-12-24T18:46:00Z

Do we really want to keep maintaining that many resolver transport modules? If there is a clear winner performance wise, why not dropping the other modules? I fail to see the advantage of providing that many options...

There can never be a clear winner since every client is different and every environment is different. We just need a small decent set.

olamy · 2022-12-25T00:09:49Z

...olver-transport-jetty/src/main/java/org/eclipse/aether/transport/jetty/JettyTransporter.java

+                            }
+                        }
+                    }
+                    httpClient.start();


is there any call to stop() to cleanup?
Shouldn't be a problem for mvn as jvm is stopped but for mvnd?
But my understanding of the code there is only one per repo?

Yes, stopping the client is missing, as noted in "Important notes" (3rd bullet). Client is created per-session, per-remote repository.

kwin · 2022-12-27T19:25:30Z

We just need a small decent set.

Exactly, for me shipping 5 HTTP transport modules is too much. That also makes the decision for consumers much harder.

cstamas · 2022-12-27T19:44:28Z

Just to clear up: the fact we have here 5 (7 to be exact) transports, that does not mean we need to deliver all these with Maven. Or in other words, transport-http, transport-file and transport-classpath was present since day 0 (of resolver existence), but no Maven included it so far. Maven 3.9/4.0 will be the first that will include transport-http.

There are 4 transports already (file, http, wagon, classpath), and this PR adds 3 new modules. But, the thing is, these are all for HTTP protocol (when protocol of remote repository is HTTP/HTTPS). These are the "old" ones:

wagon
http (aka "native")

While these are new:

java11
jetty
okhttp

New ones are all HTTP/2 capable. Personally, from new ones, I'd remove okhttp (cons: pulls in kotlin runtime and is slowest H2 client). Java11 and Jetty were really close to each other.

In short: the fact a transport is present here, does not mean we must ship it, as the case of transport-classpath and transport-http shows (they were present since day 0 of resolver, but never shipped).

kwin · 2022-12-27T19:49:15Z

Whether we ship with Maven or not is completely orthogonal to me. If they are part of resolver releases we should make sure they work properly and I don't see the capacity to maintain that many modules (nor the need TBH)

rmannibucau · 2022-12-27T19:52:26Z

My 2cts would be that Java 11 HttpClient is really worth it since it will, ultimately, enable to almost drop the resolver transitive stack when we'll be java >= 11 and go reactive (not need 256 threads to download 256 deps at the same time) but for the same reason we don't care anymore of WebDAV we shouldn't care much of http client impls (all but apache one actually?) while we have one impl which works well enough - even if it requires some system properties, we have all we need to make it work - I think it is more than sufficient and will particularly avoid the "don't use in prod until you used it and saw it was working" which does motivate much to use that.

That said, fact it does not work with mvnd should maybe be considered as a blocker - I consider mvnd as a deliverable of maven so anything targetting mvn should also target mvnd IMHO.

cstamas · 2022-12-27T20:07:34Z

It does not work currently, but #232 is here to solve that bit as well (naturally, once that is merged, this PR will need changes: to hook into those). also see apache/maven#941

cstamas · 2022-12-28T12:16:34Z

This should bump the version to 1.10 due to the build requirement change.

I agree, but let's make it in different change (as we have currently 2 PRs overlapping somewhat, this and onSessionClose). So let's make the 1.10.0 bump change in separate commit (and update JIRA)

cstamas · 2022-12-28T12:17:43Z

As nobody objected, I will drop okhttp transport, so lowering this PR to addition of 2 transports (java11 and Jetty).

cstamas · 2023-11-08T19:05:17Z

Removed this PR from 2.0.0 milestone, but am keeping it, as while main code is already on master, there may be some test related bits we will need.

cstamas · 2023-12-11T11:23:05Z

We are past this, so no reason to keep it anymore, closing it.

cstamas self-assigned this Dec 22, 2022

This comment was marked as outdated.

Sign in to view

cstamas added this to the 1.9.3 milestone Dec 22, 2022

cstamas force-pushed the http2-transports branch from 56fbf3e to 343f21b Compare December 23, 2022 14:41

cstamas force-pushed the http2-transports branch from 343f21b to 1113b62 Compare December 23, 2022 15:08

cstamas requested review from slawekjaranowski and michael-o December 23, 2022 15:19

cstamas marked this pull request as ready for review December 23, 2022 15:20

cstamas mentioned this pull request Dec 23, 2022

How to make HTTP/3 work in Jetty Client 10.x (or 11.x)? jetty/jetty.project#8974

Closed

michael-o requested changes Dec 23, 2022

View reviewed changes

olamy reviewed Dec 25, 2022

View reviewed changes

Drop OkHttp transport

d30fd49

cstamas requested review from michael-o and gnodet December 29, 2022 14:11

cstamas modified the milestones: 1.9.3, 1.10.0 Jan 5, 2023

michael-o removed their request for review January 18, 2023 19:03

cstamas mentioned this pull request Jan 27, 2023

handle thread interrupts on transitive dependencies parsing (infinite loop) #236

Closed

cstamas mentioned this pull request Feb 18, 2023

Revive the project takari/polyglot-maven#257

Merged

cstamas mentioned this pull request Mar 10, 2023

Update project airlift/resolver#25

Open

cstamas mentioned this pull request Aug 19, 2023

Drop aether-connector-okhttp dependency eclipse-m2e/m2e-core#1510

Closed

cstamas removed this from the 2.0.0 milestone Nov 8, 2023

cstamas closed this Dec 11, 2023

cstamas deleted the http2-transports branch December 11, 2023 11:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MRESOLVER-308] HTTP transport showdown. #231

[MRESOLVER-308] HTTP transport showdown. #231

cstamas commented Dec 22, 2022 •

edited

Loading

This comment was marked as outdated.

michael-o left a comment

kwin commented Dec 24, 2022

michael-o commented Dec 24, 2022

olamy Dec 25, 2022

cstamas Dec 26, 2022

kwin commented Dec 27, 2022 •

edited

Loading

cstamas commented Dec 27, 2022

kwin commented Dec 27, 2022

rmannibucau commented Dec 27, 2022

cstamas commented Dec 27, 2022

cstamas commented Dec 28, 2022

cstamas commented Dec 28, 2022 •

edited

Loading

cstamas commented Nov 8, 2023

cstamas commented Dec 11, 2023

[MRESOLVER-308] HTTP transport showdown. #231

[MRESOLVER-308] HTTP transport showdown. #231

Conversation

cstamas commented Dec 22, 2022 • edited Loading

Important notes

New Transports

Java 11 HttpClient transport

Jetty 10.x Client transport

OkHttp 4.10.x transport

This comment was marked as outdated.

michael-o left a comment

Choose a reason for hiding this comment

kwin commented Dec 24, 2022

michael-o commented Dec 24, 2022

olamy Dec 25, 2022

Choose a reason for hiding this comment

cstamas Dec 26, 2022

Choose a reason for hiding this comment

kwin commented Dec 27, 2022 • edited Loading

cstamas commented Dec 27, 2022

kwin commented Dec 27, 2022

rmannibucau commented Dec 27, 2022

cstamas commented Dec 27, 2022

cstamas commented Dec 28, 2022

cstamas commented Dec 28, 2022 • edited Loading

cstamas commented Nov 8, 2023

cstamas commented Dec 11, 2023

cstamas commented Dec 22, 2022 •

edited

Loading

kwin commented Dec 27, 2022 •

edited

Loading

cstamas commented Dec 28, 2022 •

edited

Loading