-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Gradle Enterprise Trial #4705
Gradle Enterprise Trial #4705
Conversation
…ainers/testcontainers-java into gradle-enterprise-trial
// TODO: named input and relative path for `srcDir` to make task more cacheable | ||
inputs.dir(srcDir) | ||
args(srcDir, "-d", outputDir) | ||
// TODO: `outputDir` as relative path to make task more cacheable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can improve this as a follow-up, to make caching between local-ci better.
gradle.properties
Outdated
@@ -1,3 +1,3 @@ | |||
org.gradle.parallel=false | |||
org.gradle.parallel=true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No strong opinion on this, but was recommended by the Gradle colleagues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIRC this also runs tests in parallel, and if someone attempts to build the whole project locally, they would be running a lot of heavy tests in parallel - this needs verification tho
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I discussed this with Gradle, they gave me the information, that this won't run the tests itself in one project in parallel, however, I agree that we maybe should again measure the effect on this on a full build (e.g. running all subprojects in parallel).
The way I tested it (on our CI and running only core
build) it had no negative effect.
We can also rollback this individual change for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, it won't change how tests are running in a single module, but tests from multiple modules (core, kafka, db2, ...) will run in parallel IIRC, so I would recommend reverting it (unless we verify that it isn't the case, or find a way to prevent it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed those tests could be run in parallel. It is not a real issue with the way we currently fan out CI jobs, but it still seems potentially bad for users running the whole project. So let's roll back and only activate after conscious evluation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to add a data point, I've had dev machines that were unable to run our build in parallel without also limiting the number of workers. The docker VM can often be undersized compared to the number of host machine cores, and some of the containers we run are too greedy to run side-by-side when other modules are trying to run their tests too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for sharing, definitely better to keep this off then 👍
The experiments we run did not really consider this, since the CI is set up in a way where this does not apply.
It is also interesting how more stable and reliable task caching will influence this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
alternative: turn it on on CIs only
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's leave it off, have our CI generate more data, and then we can measure the impact (i.e. improvement, if any) better of turning it on on CI.
org.gradle.caching=true | ||
|
||
org.gradle.configureondemand=true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was also recommended.
'x-amz-acl': 'public-read' | ||
] | ||
remote(HttpBuildCache) { | ||
push = isCI |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gradle colleagues recommended to always push to cache, to get a better populated cache.
We finished the trial and this could get merged to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💯 Great job!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This branch is following along with the Gradle Enterprise trial.
Keeping it in draft mode for now, but ideally, we will have a mergeable PR in the end, improving our GE integration and overall Gradle build.