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.

Sign up for GitHub

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

Jump to bottom

[MRESOLVER-231] Extend smart checksum support #140

Merged
merged 1 commit into from
Jan 31, 2022
Merged

[MRESOLVER-231] Extend smart checksum support #140

merged 1 commit into from
Jan 31, 2022

Conversation

cstamas
Copy link
Member

@cstamas cstamas commented Dec 14, 2021 •
edited
Loading

Refactor and make it pluggable. Also, make it work with Maven Central.

This change broadens "smart checksum" support in Resolver, and
gets them from HTTP response header, if present. So far only
Nexus2 was supported, but this now adds more implementations,
so for example Central is supported now as well, hence, to
download a JAR from Central will now take only 1 HTTP request
and not 2, that effectively halves count of HTTP requests done
today against central as well.

This change improves transport-http only, Wagon transport is
a big burden of layers of layers of abstraction of abstraction above
and am unsure how to do this there.

High level changes:

  • introduce ChecksumExtractor component and provide OOTB 3 implementations for it (the nexus2 was inlined, this is like "factor out nexus2" + add 2 more)
  • modify HttpTransporter to use the components
  • support nx2, nx3, artifactory, central, google central mirror as all those send inlined checksum

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

@cstamas cstamas requested review from gnodet and michael-o December 14, 2021 19:15
@cstamas cstamas self-assigned this Dec 14, 2021
@cstamas cstamas marked this pull request as ready for review December 14, 2021 19:42
@kwin
Copy link
Member

kwin commented Dec 15, 2021

This is a really nice and smart feature. Would be good to adjust the documentation at https://github.com/apache/maven-resolver/blob/master/src/site/markdown/configuration.md for aether.connector.smartChecksums as well, though.

@cstamas
Copy link
Member Author

cstamas commented Dec 16, 2021

This is a really nice and smart feature. Would be good to adjust the documentation at https://github.com/apache/maven-resolver/blob/master/src/site/markdown/configuration.md for aether.connector.smartChecksums as well, though.

Yup, I agree here. Doco is wrong, but this (Nexus2 exclusive) feature was present since eons in resolver (former Sonatype Aether), and this PR merely refactors it and makes it extensible. All former resolver releases supported this (but exclusively for Nexus2). and this PR makes it more universal, extensible.

@cstamas cstamas mentioned this pull request Dec 16, 2021
Closed
michael-o
michael-o requested changes Dec 17, 2021
View reviewed changes
src/site/markdown/smart-checksum-strategies.md Outdated Show resolved Hide resolved
src/site/markdown/smart-checksum-strategies.md Outdated Show resolved Hide resolved
src/site/markdown/smart-checksum-strategies.md Outdated Show resolved Hide resolved
src/site/markdown/smart-checksum-strategies.md Outdated Show resolved Hide resolved
@cstamas cstamas force-pushed the more-smart-checksums branch from 14ad4b2 to 9d8395d Compare January 13, 2022 15:17
@cstamas cstamas force-pushed the more-smart-checksums branch from 969e5b1 to 8c81a5a Compare January 28, 2022 07:47
@cstamas cstamas requested a review from michael-o January 28, 2022 07:48
@cstamas cstamas force-pushed the more-smart-checksums branch from 8c81a5a to 53f783d Compare January 28, 2022 08:00
michael-o
michael-o requested changes Jan 30, 2022
View reviewed changes
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.

This entire smart checksum notion needs to be named to included, since this is how it is named in the code and nothing is smart here.
The config property will be deprecated and replaced with a new one.

...r-transport-http/src/main/java/org/eclipse/aether/transport/http/HttpTransporterFactory.java Outdated Show resolved Hide resolved
...ansport-http/src/main/java/org/eclipse/aether/transport/http/XChecksumChecksumExtractor.java Show resolved Hide resolved
@cstamas cstamas force-pushed the more-smart-checksums branch from abb3899 to 116545d Compare January 31, 2022 10:32
@michael-o michael-o self-requested a review January 31, 2022 10:34
@cstamas
[MRESOLVER-231] Extend "smart checksum" feature
3a0fc02 
Generalize it and make it pluggable. Also, make it work with
major targets like Maven Central.
@cstamas cstamas force-pushed the more-smart-checksums branch from 22e421e to 3a0fc02 Compare January 31, 2022 11:10
@cstamas cstamas merged commit 9faba55 into master Jan 31, 2022
@cstamas cstamas deleted the more-smart-checksums branch January 31, 2022 11:24
@basil basil mentioned this pull request Sep 24, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kwin kwin kwin left review comments

@michael-o michael-o michael-o approved these changes

@gnodet gnodet Awaiting requested review from gnodet

Assignees

@cstamas cstamas

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@cstamas @kwin @michael-o