Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[MRESOLVER-627] Improve transport selection and logging #599

Merged
merged 7 commits into from
Nov 14, 2024

Conversation

cstamas
Copy link
Member

@cstamas cstamas commented Nov 13, 2024

In previous releases transport selection was really secretive and broken, as it considered invalid configuration as "protocol not supported" and Resolver did not log anything or worse, silently skipped to new transport.

Changes:

  • transport provider: was logging ONLY if transport selection failed (no transport was selected), changed to continuously log (in DEBUG) why a transport was ignored and next selection performed. Also, in case of total failure, exceptions are added now as suppressed ones instead just lost.
  • multiple transporter factories: smaller fixes for earlier protocol detection (file is exception to support cases like JIMFS), and when client creation is attempted but it fails due runtime exception, fail the whole process (like bad enum in JDK transport config).
  • before all transporter "blanket" threw NoTransporterEx and Basic connector was "protecting" caller from it, hence to continue doing so, no basic connector "blankets" all runtime and non-runtime exceptions coming from transport to "early report" problem as it happens in basic connector ctor.

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

In previous releases transport selection was really secretive
and broken, as it considered invalid configuration as
"protocol not supported" and Resolver did not log anything
or worse, silently skipped to new transport.

---

https://issues.apache.org/jira/browse/MRESOLVER-627
@cstamas cstamas self-assigned this Nov 13, 2024
@cstamas cstamas marked this pull request as ready for review November 13, 2024 13:03
But before all exceptions were blanketed by NoTransporterEx,
now we throw RuntimeEx as well when misconfigured.
@cstamas cstamas added this to the 2.0.4 milestone Nov 13, 2024
Copy link
Member

@michael-o michael-o left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have an opinion on this.

@cstamas
Copy link
Member Author

cstamas commented Nov 13, 2024

I don't have an opinion on this.

Sure, no problem. But let me explain what was initial cause for this PR:
We knew that JDK transport cannot handle HTTP/2 GOAWAY, and recently we saw a surge in ITs and other jobs getting it, so we decided to force HTTP/1.1 while keeping JDK transport in use. I did that by creating .mvn/maven.config in maven and mvnd projects... then I went to check "effects" using -X locally and was very surprised to see that Maven started using Apache transporter instead.

Basically, turned out my config had wrong value (a typo), and even -X did not report this, instead all I saw in debug logs is that Maven uses Apache transport.... turned out, this typo was interpreted as "not my protocol" by JDK transport, and it made Maven select next available HTTP Transporter that was Apache. Very confusing.

Hence, this PR now:

  • logs every transport "disqualification" (in DEBUG) instead of "only when no transport could be selected"
  • fails the build if any transport is mis-configured

In case of runtime ex, it is "transport configuration" issue
while in all other cases it is "plain" issue (ie no transporter
for protocol).
All those that use prioritized components.
@cstamas cstamas merged commit 5b03ce5 into apache:master Nov 14, 2024
5 checks passed
@cstamas cstamas deleted the MRESOLVER-627 branch November 14, 2024 13:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants