Skip to content
This repository has been archived by the owner on Oct 11, 2024. It is now read-only.

Speed up Exchange backup by parallelizing url fetch #1608

Merged
merged 6 commits into from
Dec 1, 2022

Conversation

meain
Copy link
Member

@meain meain commented Nov 28, 2022

Description

From rough numbers we can speedup an account with ~3000 emails, ~1000 contacts and ~1000 events from ~18m to <3m.

Type of change

  • 🌻 Feature
  • πŸ› Bugfix
  • πŸ—ΊοΈ Documentation
  • πŸ€– Test
  • πŸ’» CI/Deployment
  • 🐹 Trivial/Minor

Issue(s)

Test Plan

  • πŸ’ͺ Manual
  • ⚑ Unit test
  • πŸ’š E2E

@meain meain added the perf label Nov 28, 2022
@meain meain self-assigned this Nov 28, 2022
@meain meain temporarily deployed to Testing November 28, 2022 10:19 Inactive
@meain meain temporarily deployed to Testing November 28, 2022 10:19 Inactive
@meain meain temporarily deployed to Testing November 28, 2022 10:27 Inactive
@meain meain temporarily deployed to Testing November 28, 2022 10:27 Inactive
@meain meain temporarily deployed to Testing November 28, 2022 10:28 Inactive
@meain meain temporarily deployed to Testing November 28, 2022 10:49 Inactive
@meain meain temporarily deployed to Testing November 28, 2022 10:49 Inactive
@meain meain temporarily deployed to Testing November 28, 2022 10:49 Inactive
@meain meain temporarily deployed to Testing November 28, 2022 10:49 Inactive
@meain meain temporarily deployed to Testing November 28, 2022 10:49 Inactive
@meain meain temporarily deployed to Testing November 28, 2022 11:38 Inactive
@meain meain temporarily deployed to Testing November 28, 2022 11:38 Inactive
@meain meain temporarily deployed to Testing November 28, 2022 11:38 Inactive
@meain meain temporarily deployed to Testing November 28, 2022 11:39 Inactive
@meain meain temporarily deployed to Testing November 28, 2022 11:39 Inactive
Copy link
Contributor

@ryanfkeepers ryanfkeepers left a comment

Choose a reason for hiding this comment

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

All comments from the onedrive version of this PR also apply.

break
}
// TODO: Tweak sleep times
if i != numberOfRetries-1 {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: bit of a weird comparison. I'd rather the code make more sense, even if the const seems off by one.

Suggested change
if i != numberOfRetries-1 {
if i >= numberOfRetries {

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think i >= numberOfRetries will work. I think I'll probably change the code to the following to make it more explicit though:

for i := 1; i <= numberOfRetries; i++ {
    ...
    if i < numberOfRetries {
        time.Sleep(time.Duration(3*(i+1)) * time.Second)
    }
}

Copy link
Contributor

@ryanfkeepers ryanfkeepers Nov 29, 2022

Choose a reason for hiding this comment

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

Hah, good catch. My brain was backwards. Though, for simplicity, I'd probably go with something like:

for i := 0; i < numberOfRetries; i++ {
  response, err = query(...)
  if err == nil {
    break
  }
  time.Sleep(duration)
}

The additional conditional and fiddling with the retry count is just code noise.

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 reason for the conditional was so that it would not sleep even with an error in the last run. If we skip that, it sleeps at the end of n-1th run before exiting out of the loop.

@@ -115,19 +119,20 @@ func (col *Collection) populateByOptionIdentifier(
) {
var (
errs error
success int
success int64
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason to switch from 32 to 64?

Copy link
Member Author

Choose a reason for hiding this comment

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

I had to switch it to either int32 or int64 as I needed to do an atomic increment. And since int will be int64 on most platforms we will be deploying, I thought I would go with int64.

@@ -139,33 +144,83 @@ func (col *Collection) populateByOptionIdentifier(
return
}

limitCh := make(chan struct{}, urlPrefetchChannelBufferSize)
defer close(limitCh)
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this be added to the defer on Line 135?

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought it would be better to couple it with the make as the defer above is just for the progressbar.

Comment on lines 168 to 163
defer func() { <-limitCh }()
defer wg.Done()
Copy link
Contributor

Choose a reason for hiding this comment

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

These can be grouped.

Additionally, a comment would be helpful.

Copy link
Member Author

Choose a reason for hiding this comment

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

Any specific reason for grouping them? Let me add comments to it though.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've added comments to the channel creation.

Comment on lines +171 to +169
var (
response absser.Parsable
err error
)
Copy link
Contributor

Choose a reason for hiding this comment

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

There are several different code declarations. Could it be simplified into the top of the function?

Copy link
Member Author

@meain meain Nov 29, 2022

Choose a reason for hiding this comment

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

These are the only declarations in the func called as a goroutine.

@dadams39 dadams39 temporarily deployed to Testing November 28, 2022 18:57 Inactive
@dadams39 dadams39 temporarily deployed to Testing November 28, 2022 18:57 Inactive
@meain meain temporarily deployed to Testing November 29, 2022 06:26 Inactive
@meain meain temporarily deployed to Testing November 29, 2022 06:27 Inactive
@meain meain temporarily deployed to Testing November 29, 2022 06:27 Inactive
@meain meain temporarily deployed to Testing November 29, 2022 06:27 Inactive
@meain meain temporarily deployed to Testing November 29, 2022 07:25 Inactive
@meain meain temporarily deployed to Testing November 29, 2022 07:25 Inactive
@meain meain temporarily deployed to Testing November 29, 2022 07:25 Inactive
@meain meain temporarily deployed to Testing November 29, 2022 07:39 Inactive
@meain meain temporarily deployed to Testing November 29, 2022 07:40 Inactive
@meain meain temporarily deployed to Testing November 29, 2022 07:40 Inactive
@meain meain temporarily deployed to Testing November 29, 2022 07:41 Inactive
@meain meain temporarily deployed to Testing November 29, 2022 07:41 Inactive
@meain meain temporarily deployed to Testing November 29, 2022 07:41 Inactive
@aviator-app
Copy link
Contributor

aviator-app bot commented Dec 1, 2022

Aviator status

Aviator will automatically update this comment as the status of the PR changes.

This PR was merged using Aviator.

@aviator-app aviator-app bot temporarily deployed to Testing December 1, 2022 04:08 Inactive
@aviator-app aviator-app bot temporarily deployed to Testing December 1, 2022 04:08 Inactive
@sonarqubecloud
Copy link

sonarqubecloud bot commented Dec 1, 2022

Kudos, SonarCloud Quality Gate passed!Β  Β  Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@aviator-app aviator-app bot temporarily deployed to Testing December 1, 2022 04:09 Inactive
@aviator-app aviator-app bot temporarily deployed to Testing December 1, 2022 04:09 Inactive
@aviator-app aviator-app bot temporarily deployed to Testing December 1, 2022 04:09 Inactive
@aviator-app aviator-app bot merged commit 784db9e into main Dec 1, 2022
@aviator-app aviator-app bot deleted the exchange-speedup branch December 1, 2022 04:46
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants