Skip to content

Conversation

@cstamas
Copy link
Member

@cstamas cstamas commented Nov 1, 2022

Drop PartialFile, clean up regarding resource handling and lessen temp file use. Downside: there is no "download resume" happening anymore.

On the other hand, from now on, resolver will download "all or nothing", so no more "partial downloads" can happen either.

HttpClientTransport is STILL ABLE to resume downloads (was the only one anyway), but due removal of PartialFile it will never resume. Later we may replace removed logic with something simpler.


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

@cstamas cstamas self-assigned this Nov 1, 2022
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.

Documentation needs to be skipped as well

@cstamas cstamas requested a review from michael-o November 2, 2022 08:20
@cstamas cstamas changed the title Drop PartialFile use [MRESOLVER-282] Drop PartialFile use Nov 2, 2022
Drop all those temp files workarounds, use new utility instead.
@cstamas cstamas requested a review from michael-o November 2, 2022 09:23
@cstamas cstamas requested a review from michael-o November 2, 2022 09:52
@cstamas cstamas marked this pull request as ready for review November 2, 2022 10:06
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.

No objections, but I would expect this PR run with a custom Maven build through all Core ITs and if they pass this is good to merge.

This is HTTP Connector, while for Wagon it is alrady
true (see prev commit).
@cstamas cstamas requested review from michael-o November 2, 2022 10:30
Files.copy( is, tempFile.getPath(), StandardCopyOption.REPLACE_EXISTING );
}
try ( InputStream is = Files.newInputStream( tempFile.getPath() ) )
{
Copy link
Member

Choose a reason for hiding this comment

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

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.

All good.

Copy link
Contributor

@gnodet gnodet left a comment

Choose a reason for hiding this comment

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

Looks good.
Any particular reason to remove the resume feature ? Does it cause known problems or is that just a code clean up ?

@cstamas
Copy link
Member Author

cstamas commented Nov 8, 2022

Looks good. Any particular reason to remove the resume feature ? Does it cause known problems or is that just a code clean up ?

Both: is cleanup of ancient code, but also causes issues as devs reported (IDE + Maven got stuck in infinite loop due file lock), and after all, this code predates SyncContext that handles file locking now instead. Am unsure how much "resume download" was actually used, given it was ONLY http transport capable of (Wagon is 99% used by users and it is NOT capable of resuming).

@cstamas cstamas merged commit c4aa48d into apache:master Nov 8, 2022
@cstamas cstamas deleted the drop-partial-file branch November 8, 2022 09:16
@jira-importer
Copy link

Resolve #957

1 similar comment
@jira-importer
Copy link

Resolve #957

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.

4 participants