Skip to content

Conversation

@kennknowles
Copy link
Member

Be sure to do all of the following to help us incorporate your contribution
quickly and easily:

  • Make sure the PR title is formatted like:
    [BEAM-<Jira issue #>] Description of pull request
  • Make sure tests pass via mvn clean verify. (Even better, enable
    Travis-CI on your fork and ensure the whole test matrix passes).
  • Replace <Jira issue #> in the title with the actual Jira issue
    number, if there is one.
  • If this contribution is large, please file an Apache
    Individual Contributor License Agreement.

We can probably take this further, but this is a first step at isolating simple command-line mvn verify and mvn install from the extra artifacts that are important for release building & testing. We should keep the longer build for post-commit, maybe pre-commit, but definitely not for devs on the command line.

@kennknowles
Copy link
Member Author

R: @davorbonaci

@kennknowles kennknowles force-pushed the pom-release-profile branch from 30e8c32 to 8436056 Compare May 4, 2016 04:33
</goals>
</execution>
<execution>
<id>default-test-jar</id>
Copy link
Member

Choose a reason for hiding this comment

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

I think we need to keep this in the default profile since we have modules that depend on others modules test jars because of re-used testing utilities. Will need to factor those out to separate testing packages.

Copy link
Member Author

Choose a reason for hiding this comment

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

IMO such a dependency is a bug, for sure. Factoring out into a test utility is best, if possible. But here is a case where we cannot:

  • class A depends on SDK main
  • SDK test depends on class A
  • non-SDK test depends on class A

In this case, class A must be in SDK main. I think many things fall into this category.

Copy link
Member Author

Choose a reason for hiding this comment

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

The main use we need to think about for the long term is the way that SDKs offer their own unit tests as test suites for runners. We may refactor but for now it makes sense for everything under sdks to offer the test jar.

@lukecwik
Copy link
Member

lukecwik commented May 4, 2016

looks good so far, several other pom files that could benefit from this as well

@lukecwik
Copy link
Member

lukecwik commented May 4, 2016

R: @lukecwik

@lukecwik
Copy link
Member

lukecwik commented May 4, 2016

Did you want to limit the scope of this change to what you have here or did you want to go through the other poms?

@kennknowles
Copy link
Member Author

I'd like to get this in first. A number of these changes actually change it for everyone since they change parent pluginManagement. Submit & iterate.

@kennknowles
Copy link
Member Author

One branch of the Travis run seems to have hung downloading deps.

@lukecwik
Copy link
Member

lukecwik commented May 4, 2016

LGTM, please self merge

@kennknowles kennknowles force-pushed the pom-release-profile branch from c5a0d0a to 119812a Compare May 4, 2016 21:24
@asfgit asfgit merged commit 119812a into apache:master May 4, 2016
asfgit pushed a commit that referenced this pull request May 4, 2016
@davorbonaci
Copy link
Member

As far as I can see, this has reduced the build time from 15 minutes to 13 minutes (excluding outlays). Not a great win, IMO, with the given complexity. Perhaps we should chat separately if this is worth it.

@kennknowles
Copy link
Member Author

We should just continue to find things we can omit. Certainly compile-and-test of 13 minutes is ridiculous for this project.

@kennknowles
Copy link
Member Author

I didn't dig into too many modules, but mvn clean install -pl sdks/java/core went from ~3.5 minutes to ~2 minutes. To understand where the time goes, mvn test -pl sdks/java/core takes ~1.5 minutes.

So, taking all timings with a grain of salt, for the SDK this change reduced overall time by ~40% and reduced non-testing time by ~75%. I hope we can improve it further, and make similar improvements to all modules. We should also aim to reduce the testing time. This was a run with default settings, and it looked like parallel=none was set. Unless maven is inserting parallelism elsewhere, we should fix this.

@kennknowles kennknowles deleted the pom-release-profile branch November 10, 2016 03:10
iemejia pushed a commit to iemejia/beam that referenced this pull request Jan 12, 2018
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