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

Reduce the impact of Maven Central outages #14805

Merged
merged 2 commits into from
Oct 31, 2022

Conversation

MiguelWeezardo
Copy link
Member

Description

When setup-java action is called with maven cache, a cache entry is uploaded upon job success.

Cache keys for Maven are based on a hash of all pom.xml files in the repository.
We need to ensure that cache entries contain all Trino dependencies.

If a PR which compiles just one module M gets merged after creating a cache entry, subsequent PRs which don't change any pom.xml files will use that cache entry and will be forced to download all dependencies which weren't on M's dependency list. Not only is this slow, it also introduces a failure point since Maven Central suffers from occasional outages.

This should help with issues like #14618.

Non-technical explanation

Make sure Maven cache contains all Trino dependencies, to avoid downloading them during build.

Release notes

(x) This is not user-visible or docs only and no release notes are required.
( ) Release notes are required, please propose a release note for me.
( ) Release notes are required, with the following suggested text:

# Section
* Fix some things. ({issue}`issuenumber`)

@findepi
Copy link
Member

findepi commented Oct 27, 2022

This should help with issues like #14618.

Did you mean "fixes #14618."

Will this fix #14803 too?

.github/workflows/docs.yml Outdated Show resolved Hide resolved
@findepi findepi requested review from ebyhr and hashhar October 27, 2022 15:33
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
Copy link
Member

@hashhar hashhar left a comment

Choose a reason for hiding this comment

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

At this point I think it'd be much simpler to add a step which does mvn dependency:go-offline and use that to prepare cache instead of trying to keep track of which job produces most complete cache.

(Or add mvn dependency:go-offline to an existing job which we chose to populate cache).

@MiguelWeezardo
Copy link
Member Author

At this point I think it'd be much simpler to add a step which does mvn dependency:go-offline and use that to prepare cache instead of trying to keep track of which job produces most complete cache.

(Or add mvn dependency:go-offline to an existing job which we chose to populate cache).

I didn't know about depedency:go-offline, this is exactly what I was looking for. Thanks!

@MiguelWeezardo
Copy link
Member Author

MiguelWeezardo commented Oct 27, 2022

This should help with issues like #14618.

Did you mean "fixes #14618."

I'd rather not overpromise, since there are many failure modes this PR won't help with, and they all look like #14618

Will this fix #14803 too?

#14803 looks like a network problem in GitHub.
I don't think this PR will solve it, since it relies on GitHub network working to get cache entries.

@hashhar
Copy link
Member

hashhar commented Oct 27, 2022

@MiguelWeezardo It'd be useful to measure how much time a separate mvn dependency:go-offline JOB adds vs adding a mvn dependency:go-offline STEP in the build-pt (or some other job which always runs).

@MiguelWeezardo
Copy link
Member Author

@MiguelWeezardo It'd be useful to measure how much time a separate mvn dependency:go-offline JOB adds vs adding a mvn dependency:go-offline STEP in the build-pt (or some other job which always runs).

I misread your comment, sorry. I agree it should be faster by adding a new step not a whole job.

Copy link
Member

@hashhar hashhar left a comment

Choose a reason for hiding this comment

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

LGTM. (I did try to verify all jobs except build-pt have the "Clean local Maven repo" step but I obviously did it by eye so would appreciate someone rechecking).

@nineinchnick
Copy link
Member

BTW GitHub recently added a view for caches, when you go to the actions tab check the lower left corner:
image

@MiguelWeezardo
Copy link
Member Author

BTW GitHub recently added a view for caches, when you go to the actions tab check the lower left corner: image

Yes, and it's clear that some incomplete caches are being created (notice the size of the last entry):

setup-java-Linux-maven-39f17690b8084b8b1be072dcec83301adcf1f8463641b66daf83d0f630c0563d 910 MB cached 3 days ago
[master](https://github.com/trinodb/trino)
Last used 2 hours ago

setup-java-Linux-maven-e013dcb75512fe8010221ad18705aac972a1323919bf0cb2cc9dabdf9a37550a 890 MB cached 11 hours ago
refs/pull/14810/merge
Last used 2 hours ago

setup-java-Linux-maven-302a2568a4b5ea4116e29a722c4965334b37df5633537eb8e925fa2d88563c96 270 MB cached 4 hours ago
refs/pull/14812/merge
Last used 3 hours ago

`setup-java` action caches the local Maven repo once a job succeeds.
The cache key for Maven depends only on the hash of all pom.xml files.

By uploading caches from a job which only builds one module we are
forcing Maven to download missing dependencies during each build.
@MiguelWeezardo MiguelWeezardo force-pushed the ci_cache branch 4 times, most recently from 42cb054 to 40e946f Compare October 28, 2022 09:55
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Show resolved Hide resolved
Copy link
Member

@hashhar hashhar left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks.

When uploading a new cache entry it's important that the entry
be complete, to avoid the need of downloading more dependencies
during build.

This change explicitly downloads all dependencies in `maven-checks` job
which already builds all modules, to minimize additional work.
@nineinchnick
Copy link
Member

nineinchnick commented Oct 28, 2022

Note that downloading all dependencies adds 1.5 minutes to the maven-checks job:
image

It's not always needed - only when there was no cache hit. I think it's ok, given that:

  • it should improve the resiliency of other jobs
  • it should use the cache after this PR gets merged and when the cache rebuilds

@hashhar hashhar merged commit 527abc5 into trinodb:master Oct 31, 2022
@hashhar
Copy link
Member

hashhar commented Oct 31, 2022

Thanks @MiguelWeezardo for this.

@github-actions github-actions bot added this to the 402 milestone Oct 31, 2022
@MiguelWeezardo MiguelWeezardo deleted the ci_cache branch October 31, 2022 12:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

4 participants