Remove shared `getDockerBuildTask` to enable parallel docker builds #16384

evantahler · 2022-09-06T23:05:06Z

After a hot tip from the creator of the Docker Gradle plugin we are using, I removed our shared getDockerBuildTask gradle task and moved it into each project. This did allow our docker builds to run in parallel!
* Before - 18:30
* After - 8:40

~~Note how in the build scans the docker tasks in "after" ran in parallel 🥳~~

To achieve ^ cleanly, we define logic in root build.gradle to create a task in each subproject if the subproject contains dockerImageName in the gradle.properties file.

Some caveats:

we also define a copyGeneratedTar task that is applied to all subprojects with the dockerImageName property. Although this do not need to be used in each subproject.
we leave it up to each subproject to define what task is depends on since some projects have more custom copy logic.

evantahler · 2022-09-06T23:06:09Z

@davinchia I tried to sort out how we might share this Gradle method outside of the main build.gradle file (https://stackoverflow.com/questions/18715137/extract-common-methods-from-gradle-build-script#18718802) and I really can't get anything to work... any tips?

davinchia · 2022-09-08T00:46:06Z

Some notes:

I think this is happening because we are lazily creating a task, which is not evaluated until Gradle is in the execution phase. Because of this, Gradle doesn't know it can parallelise the task.
I think the way around this is to create a task in the buildSrc that isn't lazily evaluated. Didn't have time today to look into that.

evantahler · 2022-09-08T17:08:05Z

@davinchia I've hit the end of my Gradle abilities (I tried messing around in buildSrc for a while). I've cleaned things up a bit, but there's still a lot of repeated code.

I suggest we merge this in now, so we get the speed-up, and then in the future when you've got some time, a refactor can take place.

davinchia · 2022-09-08T17:09:37Z

@evantahler i'm pairing with @jdpgrailsdev later. I want to spend some time tomorrow trying something before we merge this in. That sound ok?

davinchia · 2022-09-09T04:13:36Z

airbyte-config/init/build.gradle

+    buildArgs.put('JDK_IMAGE', openjdkImage)
+    buildArgs.put('VERSION', buildVersion)
+}
+localDockerBuildTask.dependsOn(copyScripts)


@jdpgrailsdev I believe what we did today works!

One hiccup is we have submodules whose copy tasks are on scripts instead of the generated tar. I have some idea on how to fix this but want to do in person as it's faster.

evantahler · 2022-09-09T16:35:47Z

build.gradle

+    // If subprojects have the dockerImageName property configured in their gradle property file,
+    // register:
+    // 1) A task to copy the generated tar.
+    // 2) A task to build the docker image that depends on the generated tar.
+    // 3) Make the docker image task depend on the default assemble task.


;

git-phu

looks great!

the old def Task getDockerBuildTask always felt like a hack anyway

evantahler · 2022-09-10T00:27:42Z

Thanks for getting this done @davinchia!

davinchia · 2022-09-10T00:53:44Z

docs/contributing-to-airbyte/developing-on-docker.md

-method takes 2 arguments:
- The image name, for example if `foo` is given as an image name, the image `airbyte/foo` will be created
- The project directory folder. It is needed because the `getDockerBuildTask` is declared in the rootProject
+The top level `build.gradle` file defines several convenient tasks for building a docker image.


@git-phu how does this look?

looks great! Just one comment

git-phu

looks great! Just one comment

docs/contributing-to-airbyte/developing-on-docker.md

git-phu · 2022-09-10T01:03:38Z

docs/contributing-to-airbyte/developing-on-docker.md

-method takes 2 arguments:
- The image name, for example if `foo` is given as an image name, the image `airbyte/foo` will be created
- The project directory folder. It is needed because the `getDockerBuildTask` is declared in the rootProject
+The top level `build.gradle` file defines several convenient tasks for building a docker image.


looks great! Just one comment

Co-authored-by: Peter Hu <peter@airbyte.io>

subodh1810 · 2022-09-15T19:30:55Z

@evantahler @davinchia I think this PR broke my local build, for all the gradle modules which dont have the application plugin configured (Applying the Application plugin also implicitly applies the Distribution plugin), the task distTar is unknown and since the new subProject logic makes the copyGeneratedTar task depend on the task distTar (which is not available), the build is fails. I dont know if its just me or others are facing this as well and how is this working on the CI.

    if (subproj.hasProperty('dockerImageName')) {
        project.logger.lifecycle("configuring docker task for $subproj.name")

        // Although this task is defined for every subproject with the dockerImageName property,
        // It is not necessarily used for all subprojects. Non-TAR producing projects can ignore this.
        tasks.register("copyGeneratedTar", Copy) {
            dependsOn copyDocker
            dependsOn distTar

The error that am getting (this is just for airbyte-cli but the same error pops up for all the modules which dont have the application plugin)

Could not create task ':airbyte-cli:copyGeneratedTar'.
> Could not get unknown property 'distTar' for task ':airbyte-cli:copyGeneratedTar' of type org.gradle.api.tasks.Copy.

I fixed it by adding the distribution plugin to all such modules (see PR #16781) , but am not sure if thats the right thing to do. I think we might need to re-tweak the logic of copyGeneratedTar and not make it depend on the distTar task for all the modules.

Let me know what you think of it.

davinchia · 2022-09-15T21:13:32Z

@subodh1810 this isn't showing up for me or on the build runners. Wonder if it's specific to you. Can you try ./gradlew clean build to see if that fixes it?

…irbytehq#16384) Define logic in root build.gradle to create a task in each subproject if the subproject contains dockerImageName in the gradle.properties file. Some caveats: - We also define a copyGeneratedTar task that is applied to all subprojects with the dockerImageName property. - This does not need to be used in each subproject. We leave it up to each subproject to define what task is depends on since some projects have more custom copy logic.

Remove shared getDockerBuildTask to enable parallel docker builds

7dee0e9

evantahler requested a review from davinchia September 6, 2022 23:05

github-actions bot added area/platform issues related to the platform area/server area/worker Related to worker area/frontend Related to the Airbyte webapp labels Sep 6, 2022

remove build method

3057bb5

Merge branch 'master' into evan/gradle-docker-speedup

b2d8577

evantahler marked this pull request as ready for review September 8, 2022 17:08

evantahler requested review from a team as code owners September 8, 2022 17:08

evantahler temporarily deployed to more-secrets September 8, 2022 17:10 Inactive

evantahler self-assigned this Sep 8, 2022

evantahler added the team/platform-move label Sep 8, 2022

Merge branch 'master' into evan/gradle-docker-speedup

796d6a9

evantahler temporarily deployed to more-secrets September 8, 2022 22:36 Inactive

Use better script pattern in all modules that need it.

aaf2373

github-actions bot added the area/documentation Improvements or additions to documentation label Sep 9, 2022

davinchia temporarily deployed to more-secrets September 9, 2022 04:07 Inactive

Add back bad import.

5c8ceb7

davinchia temporarily deployed to more-secrets September 9, 2022 04:10 Inactive

Add comments.

864e698

davinchia reviewed Sep 9, 2022

View reviewed changes

davinchia temporarily deployed to more-secrets September 9, 2022 04:14 Inactive

evantahler commented Sep 9, 2022

View reviewed changes

Modify this to work for metrics reporter.

5610fb5

davinchia added 3 commits September 9, 2022 14:16

Remove unneeded change.

e7a7089

Remove unneeded change.

851c58e

Remove accidentally added change.

5e04881

github-actions bot removed the area/documentation Improvements or additions to documentation label Sep 9, 2022

davinchia temporarily deployed to more-secrets September 9, 2022 21:19 Inactive

Update comments in root build.gradle.

82e9e99

;

davinchia temporarily deployed to more-secrets September 9, 2022 21:26 Inactive

Update docker task docs.

a64aa54

github-actions bot added the area/documentation Improvements or additions to documentation label Sep 9, 2022

davinchia temporarily deployed to more-secrets September 9, 2022 21:34 Inactive

davinchia requested review from git-phu and jdpgrailsdev September 10, 2022 00:18

git-phu approved these changes Sep 10, 2022

View reviewed changes

davinchia requested a review from sherifnada September 10, 2022 00:41

Update documentation.

eeead40

davinchia reviewed Sep 10, 2022

View reviewed changes

davinchia temporarily deployed to more-secrets September 10, 2022 00:54 Inactive

git-phu approved these changes Sep 10, 2022

View reviewed changes

Update docs/contributing-to-airbyte/developing-on-docker.md

0b8dfbe

Co-authored-by: Peter Hu <peter@airbyte.io>

davinchia merged commit 834ac1a into master Sep 10, 2022

davinchia deleted the evan/gradle-docker-speedup branch September 10, 2022 01:40

davinchia temporarily deployed to more-secrets September 10, 2022 01:42 Inactive

octavia-squidington-iii mentioned this pull request Sep 12, 2022

Bump Airbyte version from 0.40.4 to 0.40.5 #16610

Merged

subodh1810 mentioned this pull request Sep 15, 2022

add distribution plugin for modules using copyGeneratedTar to fix build #16781

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove shared `getDockerBuildTask` to enable parallel docker builds #16384

Remove shared `getDockerBuildTask` to enable parallel docker builds #16384

evantahler commented Sep 6, 2022 •

edited by davinchia

evantahler commented Sep 6, 2022 •

edited

davinchia commented Sep 8, 2022

evantahler commented Sep 8, 2022

davinchia commented Sep 8, 2022

davinchia Sep 9, 2022

evantahler Sep 9, 2022

git-phu left a comment

evantahler commented Sep 10, 2022

davinchia Sep 10, 2022

git-phu Sep 10, 2022

davinchia Sep 10, 2022

git-phu left a comment

git-phu Sep 10, 2022

subodh1810 commented Sep 15, 2022 •

edited

davinchia commented Sep 15, 2022

Remove shared getDockerBuildTask to enable parallel docker builds #16384

Remove shared getDockerBuildTask to enable parallel docker builds #16384

Conversation

evantahler commented Sep 6, 2022 • edited by davinchia

evantahler commented Sep 6, 2022 • edited

davinchia commented Sep 8, 2022

evantahler commented Sep 8, 2022

davinchia commented Sep 8, 2022

davinchia Sep 9, 2022

Choose a reason for hiding this comment

evantahler Sep 9, 2022

Choose a reason for hiding this comment

git-phu left a comment

Choose a reason for hiding this comment

evantahler commented Sep 10, 2022

davinchia Sep 10, 2022

Choose a reason for hiding this comment

git-phu Sep 10, 2022

Choose a reason for hiding this comment

davinchia Sep 10, 2022

Choose a reason for hiding this comment

git-phu left a comment

Choose a reason for hiding this comment

git-phu Sep 10, 2022

Choose a reason for hiding this comment

subodh1810 commented Sep 15, 2022 • edited

davinchia commented Sep 15, 2022

Remove shared `getDockerBuildTask` to enable parallel docker builds #16384

Remove shared `getDockerBuildTask` to enable parallel docker builds #16384

evantahler commented Sep 6, 2022 •

edited by davinchia

evantahler commented Sep 6, 2022 •

edited

subodh1810 commented Sep 15, 2022 •

edited