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

Parallelize maven builds by default #22694

Merged

Conversation

ZacBlanco
Copy link
Contributor

@ZacBlanco ZacBlanco commented May 8, 2024

Description

Adding these parameters defaults all maven builds from this directory to using a parallelized mode if running on a multi-core machine. It can significantly improve build performance when compiling multiple modules.

Motivation and Context

Faster builds without having to write -T 1C for every maven command.

Impact

Potentially faster builds. Console output may be intertwined which can make following errors more difficult

Test Plan

N/A

Contributor checklist

  • Please make sure your submission complies with our development, formatting, commit message, and attribution guidelines.
  • PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
  • Documented new properties (with its default value), SQL syntax, functions, or other functionality.
  • If release notes are required, they follow the release notes guidelines.
  • Adequate tests were added if applicable.
  • CI passed.

Release Notes

== NO RELEASE NOTE ==

@ZacBlanco ZacBlanco requested a review from a team as a code owner May 8, 2024 15:02
@ZacBlanco ZacBlanco requested a review from presto-oss May 8, 2024 15:02
@ZacBlanco ZacBlanco force-pushed the upstream-default-maven-parallelism branch from d4da795 to 8c36b0c Compare May 10, 2024 15:15
@ZacBlanco
Copy link
Contributor Author

cc: @aaneja

Copy link
Contributor

@elharo elharo left a comment

Choose a reason for hiding this comment

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

The build has several warnings about non-parallelizable plugins. Anything we should do about that?

@ZacBlanco
Copy link
Contributor Author

Thanks for bringing that up. As far as I'm aware, the plugins don't seem to fail when executing. I've personally never seen a build failure that would have been due to that issue.

I combed through the build and this is the list of plugins currently marked as "not parallelizable".

  • presto-maven-plugin
    • a custom plugin found at github.com/prestodb/presto-maven-plugin . I didn't find any unsafe code browsing the source. I think it would require a new metadata update + plugin release to fix
  • provisio-maven-plugin
    • a dependency of the presto-maven-plugin. We would need to upgrade the presto-maven-plugin to remove this warning
  • antlr4-maven-plugin
    • A dependency upgrade fixes this, but the latest versions require Java 11+. We don't officially build with Java 11 yet
  • build-helper-maven-plugin
    • recent versions are marked as thread safe. I think we should be able to just bump the version, but I don't see the declarations for it in our pom.xml.
  • drift-maven-plugin
    • I couldn't find the source for the current plugin version.
  • fmpp-maven-plugin
    • An old and unmainted plugin that is probably thread-safe based on the source, but would require a new patch version+release.

Overall, the main effort with removing these warnings would be contacting the maintainers and getting them to issue a patch release with the plugin metadata updated to mark it as thread safe. I think many developers already set the -T argument on their machines and I know for a fact our internal build system already uses the parallel builds. I think it should be safe to ignore the plugin warnings.

@tdcmeehan tdcmeehan self-assigned this May 14, 2024
aaneja
aaneja previously approved these changes May 17, 2024
Adding these parameters defaults all maven builds from this
directory to using a parallelized mode if running on a multi-core
machine. It can significantly improve build performance when
compiling multiple modules.
@ZacBlanco ZacBlanco force-pushed the upstream-default-maven-parallelism branch from 9a4aca9 to 1e402b8 Compare May 22, 2024 16:55
@ZacBlanco ZacBlanco merged commit cf0ac07 into prestodb:master May 22, 2024
56 checks passed
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.

None yet

4 participants