Skip to content

Add forkCount option to parallelize build and tests#2278

Merged
sijie merged 3 commits intoapache:masterfrom
Ghatage:parallelizeBuildAndTests
Apr 21, 2020
Merged

Add forkCount option to parallelize build and tests#2278
sijie merged 3 commits intoapache:masterfrom
Ghatage:parallelizeBuildAndTests

Conversation

@Ghatage
Copy link
Contributor

@Ghatage Ghatage commented Mar 5, 2020

Motivation

Current code forks a new JVM per module. (bookkeeper-server, bookkeeper-proto etc)
This means one fork per module for build and testing, no parallelism within the module where majority of the time goes.
We need parallelism within a module during the test execution so we can have the builds complete faster and have the artifacts shipped out quicker.

Changes

We use the maven surefire plugin but don't define the forkCount and hence set it to default of 1.
This means it executes each module with one thread.
This change sets forkCount to 5, enabling parallelism in testing and drastically reducing total turnaround time. (by about 2/3rds!)

Total build+test time without this change

[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  01:06 h
[INFO] Finished at: 2020-03-05T02:01:29-08:00
[INFO] ------------------------------------------------------------------------

Total build+test time with this change

[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  18:23 min
[INFO] Finished at: 2020-03-05T02:38:22-08:00
[INFO] ------------------------------------------------------------------------

Things to watch

Added parallelism may cause some flappers but with much trial and error I have come to the number 5. The flappers are usually only from conflict in obtaining the same port number.
If needed, we can increase the retryCount, but as of now I consistently don't see any flappers at a forkCount of 5

@Ghatage
Copy link
Contributor Author

Ghatage commented Mar 5, 2020

@eolivelli Looks like we've halved the total execution time required to run most CI tests on Github Actions! (it's even better locally, on my MBP its reduced by 2/3rds)
Can you please re-run the flapper? (Since this is Github actions only committers can re-execute the tests)

Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

Sorry
I missed the notifications.

LGTM.
I have restarted GitHub tests

Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

Re-thinking about this change....

can you please add a build property ?
this way the developer can change the parallelism.
<forkCount>${forkCount}</forkCount>

Copy link
Contributor

@eolivelli eolivelli 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 have restarted failed jobs.
I sincerely hope that this change won't add flakyness to GitHub Actions jobs.

Please @Ghatage keep an eye these days on GitHub Action jobs,
we can add -DforkCount.variable=1 (or 2) in github workflows configurations

@Ghatage
Copy link
Contributor Author

Ghatage commented Mar 25, 2020

Will do. We can reduce it as you suggest if it causes too much ruckus.

@eolivelli
Copy link
Contributor

I have restarted bookie tests and integration tests.
Is this patch affecting integration tests ?
I think we cannot run more than one IT at a time, it will use many resources

@Ghatage
Copy link
Contributor Author

Ghatage commented Mar 31, 2020

Fair enough @eolivelli. I have moved it to 1.
Please note that folks will be able to take benefit of this addition by changing <forkCount.variable> locally in their pom.xml
Hopefully his should help the Github Actions Integration tests.

@eolivelli
Copy link
Contributor

I have restarted the failing Bookie Tests

@sijie sijie added this to the 4.11.0 milestone Apr 21, 2020
@sijie sijie merged commit e20a24a into apache:master Apr 21, 2020
Ghatage added a commit to Ghatage/bookkeeper that referenced this pull request Jun 19, 2020
### Motivation
Current code forks a new JVM per module. (bookkeeper-server, bookkeeper-proto etc)
This means one fork per module for build and testing, no parallelism within the module where majority of the time goes.
We need parallelism within a module during the test execution so we can have the builds complete faster and have the artifacts shipped out quicker.

### Changes
We use the maven surefire plugin but don't define the `forkCount` and hence set it to default of 1.
This means it executes each module with one thread.
This change sets `forkCount` to 5, enabling parallelism in testing and drastically reducing total turnaround time. (by about 2/3rds!)

*Total build+test time without this change*
```
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  01:06 h
[INFO] Finished at: 2020-03-05T02:01:29-08:00
[INFO] ------------------------------------------------------------------------
```
*Total build+test time with this change*
```
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  18:23 min
[INFO] Finished at: 2020-03-05T02:38:22-08:00
[INFO] ------------------------------------------------------------------------
```

### Things to watch
Added parallelism may cause some flappers but with much trial and error I have come to the number `5`. The flappers are usually only from conflict in obtaining the same port number.
If needed, we can increase the retryCount, but as of now I consistently don't see any flappers at a `forkCount` of 5

Reviewers: Enrico Olivelli <eolivelli@gmail.com>, Sijie Guo <None>

This closes apache#2278 from Ghatage/parallelizeBuildAndTests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants