-
Notifications
You must be signed in to change notification settings - Fork 73
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
Parallelise some tasks #1434
Parallelise some tasks #1434
Conversation
Generate changelog in
|
@@ -40,6 +41,7 @@ | |||
* plugin <a href="https://github.com/palantir/gradle-baseline/pull/1944">gradle-baseline#1944</a>. | |||
*/ | |||
final class ModuleArgs { | |||
private static final Logger log = Logging.getLogger(ModuleArgs.class); |
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.
does gradle route this tot he project logger here? not sure how it would :S
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.
Unfortunately there's no injectable logger you can use. This is the only real ways I found. gradle/gradle#2678
@@ -40,7 +42,11 @@ public static Path replaceVars(InputStream src, Path dest, Map<String, String> v | |||
dest.getParent().toFile().mkdirs(); | |||
|
|||
// write content | |||
return Files.write(dest, text.getBytes(StandardCharsets.UTF_8)); | |||
try { |
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.
I was thinking, what if we relaxed the "doesn't throw exception" constraint? if we were writing tasks normally, with the @TaskAction
we could just slap a throws Exception
on the end and everything would be fine. In the generated task, we just wrap invocations with a try catch? thoughts?
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.
only because it just makes code a bit bloated and distracting / or you have to make a million "unsafe write" type methods
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.
I think the only downside would be another layer of rethrown exception in the stacktrace that does not have any useful information.
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.
I guess the RuntimeException I've made below is not really that useful either. But the autowrapper exception would be something like "Task LaunchConfigTask failed"
looks good 👍 |
Released 7.28.0 |
Before this PR
None of the custom task types in this repo use the worker api - this means they cannot be parallelised within the same project and even worse, they can hold up longer parallelisable tasks (like
distTar
) from being run in parallel.After this PR
Use auto-parallelizable to make some of these tasks run in parallel.
==COMMIT_MSG==
createLaunchConfig
,mergeDiagnosticsJson
,createInitScript
andcreateCheckScript
can all now run in parallel within the same project.==COMMIT_MSG==
Possible downsides?