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

Optimize build time #1098

Merged
merged 40 commits into from
Jul 25, 2024
Merged

Conversation

chandrashekar-s
Copy link
Collaborator

@chandrashekar-s chandrashekar-s commented Jun 19, 2024

Description of what I changed

The PR contains changes to optimize the cloud built. Certain steps are triggered to run parallel and few changes in maven build to avoid unnecessary creation of javadoc and source jars.

The below flow chart contains the the sequence in which the steps are executed.

cloudbuild_flowChart drawio

E2E test

Relied on E2E tests.

TESTED:

NA

Checklist: I completed these to help reviewers :)

  • I have read and will follow the review process.

  • I am familiar with Google Style Guides for the language I have coded in.

    No? Please take some time and review Java and Python style guides.

  • My IDE is configured to follow the Google code styles.

    No? Unsure? -> configure your IDE.

  • I have added tests to cover my changes. (If you refactored existing code that was well tested you do not have to add tests)

  • I ran mvn clean package right before creating this pull request and added all formatting changes to my commit.

  • All new and existing tests passed.

  • My pull request is based on the latest changes of the master branch.

    No? Unsure? -> execute command git pull --rebase upstream master

@codecov-commenter
Copy link

codecov-commenter commented Jun 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 51.80%. Comparing base (022c3fd) to head (6f990fd).

Additional details and impacted files
@@             Coverage Diff              @@
##             master    #1098      +/-   ##
============================================
+ Coverage     51.78%   51.80%   +0.01%     
- Complexity      669      670       +1     
============================================
  Files            95       95              
  Lines          5577     5577              
  Branches        724      724              
============================================
+ Hits           2888     2889       +1     
  Misses         2410     2410              
+ Partials        279      278       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@chandrashekar-s chandrashekar-s requested a review from bashir2 June 24, 2024 07:31
@chandrashekar-s
Copy link
Collaborator Author

Hi @bashir2, the changes are ready for review. With the above changes the build time has reduced from 60 minutes to 45 minutes approximately. The most time consuming steps which are still left are Bring up controller and Spark containers and Run E2E Test for Dockerized Controller and Spark Thriftserver. For the Run E2E Test for Dockerized Controller and Spark Thriftserver step changing to process only the resources Patient,Encounter and Observation only reduced the time by 3 minutes (reduced from 13 minutes to 10 minutes), so I have not included this change.

Please ignore the multiple commits as I tested the changes via the PR.

Copy link
Collaborator

@bashir2 bashir2 left a comment

Choose a reason for hiding this comment

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

Thanks @chandrashekar-s for speeding up the build process. My comments are mostly minor. Also, I liked the flowchart you have in the PR description. Can you please add that to developers.md and also document what tool you used to create it?

cloudbuild.yaml Outdated Show resolved Hide resolved
cloudbuild.yaml Outdated Show resolved Hide resolved
docker/sink-compose1.yml Outdated Show resolved Hide resolved
@chandrashekar-s chandrashekar-s force-pushed the optimise-build-time branch 2 times, most recently from b449539 to 6ac6c58 Compare July 9, 2024 03:54
Copy link
Collaborator

@bashir2 bashir2 left a comment

Choose a reason for hiding this comment

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

Thanks for the changes @chandrashekar-s, please feel free to merge after addressing minor comments; also a reminder of having one commit per review round as mentioned in the review process :-)

cloudbuild.yaml Outdated Show resolved Hide resolved
cloudbuild.yaml Outdated Show resolved Hide resolved
cloudbuild.yaml Outdated Show resolved Hide resolved
cloudbuild.yaml Outdated Show resolved Hide resolved
doc/cloudbuild_flowchart.xml Outdated Show resolved Hide resolved
@chandrashekar-s chandrashekar-s force-pushed the optimise-build-time branch 4 times, most recently from ac981e3 to 4d16589 Compare July 24, 2024 11:40
@chandrashekar-s chandrashekar-s force-pushed the optimise-build-time branch 6 times, most recently from 40de27f to 3614843 Compare July 25, 2024 07:26
@chandrashekar-s
Copy link
Collaborator Author

The build time has been reduced from 1 hour to 40 minutes now with this PR. There is scope for more optimization as listed in this issue. For now, this PR will be closed.

@chandrashekar-s
Copy link
Collaborator Author

@bashir2 I will be closing this PR now, please let me know if you see any concerns in the latest commits.

@chandrashekar-s chandrashekar-s merged commit 77d1649 into google:master Jul 25, 2024
6 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.

3 participants