-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
feat(reposerver): Skip calling git fetch if commit to checkout exists locally #18657
feat(reposerver): Skip calling git fetch if commit to checkout exists locally #18657
Conversation
Signed-off-by: Shady Rafehi <shady@canva.com>
Signed-off-by: Shady Rafehi <shady@canva.com>
Signed-off-by: Shady Rafehi <shady@canva.com>
af3eef6
to
566ea73
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #18657 +/- ##
==========================================
+ Coverage 47.56% 47.58% +0.01%
==========================================
Files 332 332
Lines 46746 46771 +25
==========================================
+ Hits 22236 22256 +20
Misses 21665 21665
- Partials 2845 2850 +5 ☔ View full report in Codecov by Sentry. |
Co-authored-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> Signed-off-by: Shady Rafehi <shady@canva.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @shady-canva. The change LGTM 🚀
@crenshaw-dev do you have any other concerns?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, let's see how it behaves!
Awesome, thanks for this! |
Awesome 👏 Will this backport to the previous versions? |
… locally (argoproj#18657) * Skip fetch if revision to check out exists locally Signed-off-by: Shady Rafehi <shady@canva.com> * Test reposerver/repository.checkoutRevision Signed-off-by: Shady Rafehi <shady@canva.com> * Test client.IsRevisionPresent Signed-off-by: Shady Rafehi <shady@canva.com> * Signoff Signed-off-by: Shady Rafehi <shady@canva.com> * Update reposerver/repository/repository.go Co-authored-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> Signed-off-by: Shady Rafehi <shady@canva.com> --------- Signed-off-by: Shady Rafehi <shady@canva.com> Co-authored-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>
This is much appreciated! 🙌 |
thanks for this 🙌 |
… locally (argoproj#18657) * Skip fetch if revision to check out exists locally Signed-off-by: Shady Rafehi <shady@canva.com> * Test reposerver/repository.checkoutRevision Signed-off-by: Shady Rafehi <shady@canva.com> * Test client.IsRevisionPresent Signed-off-by: Shady Rafehi <shady@canva.com> * Signoff Signed-off-by: Shady Rafehi <shady@canva.com> * Update reposerver/repository/repository.go Co-authored-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> Signed-off-by: Shady Rafehi <shady@canva.com> --------- Signed-off-by: Shady Rafehi <shady@canva.com> Co-authored-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>
… locally (argoproj#18657) * Skip fetch if revision to check out exists locally Signed-off-by: Shady Rafehi <shady@canva.com> * Test reposerver/repository.checkoutRevision Signed-off-by: Shady Rafehi <shady@canva.com> * Test client.IsRevisionPresent Signed-off-by: Shady Rafehi <shady@canva.com> * Signoff Signed-off-by: Shady Rafehi <shady@canva.com> * Update reposerver/repository/repository.go Co-authored-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> Signed-off-by: Shady Rafehi <shady@canva.com> --------- Signed-off-by: Shady Rafehi <shady@canva.com> Co-authored-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> Signed-off-by: Javier Solana <javier.solana@cabify.com> Signed-off-by: Javier Solana <javier.solana@cabify.com>
Problem
Our monorepo is frequently updated with hundreds of application changes in a very short period of time (within 15 to 30 minutes). This leads to a large amount of calls to checkoutRevision in the reposerver, where
git fetch
is called each time. SincecheckoutRevision
is wrapped in a lock, it becomes a bottleneck that causes the application controller's work-queue depth to blow up:Solution
When we have a lot of commits landing around the same time, chances are a previous
git fetch
would have already pulled in the commit SHA we want to checkout. Therefore, this PR introduces a check to see if the revision being checked out already exists locally. If so,git fetch
is skipped and we move on directly togit checkout
.Outcome
Decreased
git fetch
callsWe've had this change deployed in production for a month now, and so far we've seen an 80% decrease in
git fetch
calls. Over a two week period, we ended up skipping 134,000 calls we would have otherwise made:Decreased work-queue depth
Our application-controller has been a lot healthier since we made this change. During most peak periods, we only see a work-queue depth in the single digits now. Previously we were dealing with a depth exceeding 400.
Decreased sync times
Our application sync durations are now steady and predictable, usually in the range of 20 to 30 seconds, no matter the load. Prior to the change, we were dealing with sync durations in the 5 to 10 minute range.
Checklist: