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.

Sign up for GitHub

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

Jump to bottom

Refactor chunk upload and make the parallel uploads configurable #68

Merged
merged 3 commits into from
Nov 28, 2018
Merged

Refactor chunk upload and make the parallel uploads configurable #68

merged 3 commits into from
Nov 28, 2018

Conversation

swapnilgm
Copy link
Contributor

@swapnilgm swapnilgm commented Nov 19, 2018 •
edited
Loading

Signed-off-by: Swapnil Mhamane [email protected]

What this PR does / why we need it:
Earlier we used to spawn go routine for all chunks simultaneously which was causing network traffic. This PR refactor the multi-part/chunk upload code. Now, we can configure the number of parallel chunk upload upper limit.
Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:
Please test both snapshot and server sub-command against all cloud providers.
Release note:

Now, snapshot upload happens in chunk. One can configure the number of parallel chunk uploads by setting command line argument `max-parallel-chunk-uploads`. Default is set to 5.

@swapnilgm swapnilgm added kind/enhancement Enhancement, improvement, extension size/s Size of pull request is small (see gardener-robot robot/bots/size.py) component/etcd-backup-restore ETCD Backup & Restore exp/intermediate Issue that requires some project experience platform/all priority/normal needs/tests Needs (more) tests needs/lgtm Needs approval for merging area/backup Backup related needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) status/new Issue is new and unprocessed labels Nov 19, 2018
@swapnilgm swapnilgm force-pushed the fix/multi-part-upload branch from 24a1b34 to abaf3c5 Compare November 21, 2018 10:51
@swapnilgm swapnilgm force-pushed the fix/multi-part-upload branch from abaf3c5 to 40558fa Compare November 26, 2018 11:48
@swapnilgm
Copy link
Contributor Author

@georgekuruvillak @shreyas-s-rao Thank you for the review. I have addressed your comments. PTAL.

georgekuruvillak
georgekuruvillak reviewed Nov 27, 2018
View reviewed changes
pkg/snapstore/utils.go Outdated
default:
chunkUploadCh <- *chunk
}
})
}
remainingChunks--
Copy link
Contributor

Choose a reason for hiding this comment

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

remainingChunks should be reduced only on a successful upload, ryt? So can we have it in an else?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh. Right. Sorry this I probably changed back in second iteration of refactoring. Will change it.

georgekuruvillak
georgekuruvillak reviewed Nov 27, 2018
View reviewed changes
Copy link
Contributor

@georgekuruvillak georgekuruvillak 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 work. Well written. I have some minor comments. Can you go through it and address it?

georgekuruvillak
georgekuruvillak suggested changes Nov 27, 2018
View reviewed changes
Copy link
Contributor

@georgekuruvillak georgekuruvillak 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 work. Well written. I have some minor comments. Can you go through it and address it?

@swapnilgm swapnilgm force-pushed the fix/multi-part-upload branch from 680e421 to 636fd53 Compare November 27, 2018 09:32
@swapnilgm
Copy link
Contributor Author

swapnilgm commented Nov 27, 2018 •
edited
Loading

@georgekuruvillak Done. Tested and updated PR. Automated negative unit test are on the way in different PR once we have mock clients for different providers.

@swapnilgm swapnilgm added status/in-progress Issue is in progress/work and removed status/new Issue is new and unprocessed needs/tests Needs (more) tests labels Nov 27, 2018
@gardener-robot-ci-1 gardener-robot-ci-1 added needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Nov 27, 2018
shreyas-s-rao
shreyas-s-rao requested changes Nov 28, 2018
View reviewed changes
Copy link
Collaborator

@shreyas-s-rao shreyas-s-rao 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 PR! I just have a few refactoring suggestions. Other than that, LGTM! 😃

pkg/snapstore/abs_snapstore.go Outdated Show resolved Hide resolved
pkg/snapstore/types.go Outdated Show resolved Hide resolved
pkg/snapstore/gcs_snapstore.go Outdated Show resolved Hide resolved
pkg/snapstore/swift_snapstore.go Outdated Show resolved Hide resolved
@swapnilgm swapnilgm force-pushed the fix/multi-part-upload branch from 636fd53 to ff934a8 Compare November 28, 2018 10:46
@georgekuruvillak georgekuruvillak added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) reviewed/lgtm Has approval for merging and removed needs/lgtm Needs approval for merging needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Nov 28, 2018
@gardener-robot-ci-1 gardener-robot-ci-1 added needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Nov 28, 2018
@swapnilgm swapnilgm force-pushed the fix/multi-part-upload branch from ff934a8 to a19ec64 Compare November 28, 2018 13:28
@swapnilgm swapnilgm added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Nov 28, 2018
@gardener-robot-ci-1 gardener-robot-ci-1 added needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Nov 28, 2018
@swapnilgm swapnilgm added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) size/m Size of pull request is medium (see gardener-robot robot/bots/size.py) and removed needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) size/s Size of pull request is small (see gardener-robot robot/bots/size.py) labels Nov 28, 2018
@gardener-robot-ci-1 gardener-robot-ci-1 added needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Nov 28, 2018
shreyas-s-rao
shreyas-s-rao approved these changes Nov 28, 2018
View reviewed changes
Copy link
Collaborator

@shreyas-s-rao shreyas-s-rao left a comment

Choose a reason for hiding this comment

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

LGTM. 👍🏼

@swapnilgm swapnilgm merged commit 3cbb0ba into gardener:master Nov 28, 2018
@swapnilgm swapnilgm deleted the fix/multi-part-upload branch November 28, 2018 17:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shreyas-s-rao shreyas-s-rao shreyas-s-rao approved these changes

@georgekuruvillak georgekuruvillak georgekuruvillak approved these changes

Assignees
No one assigned
Labels
area/backup Backup related component/etcd-backup-restore ETCD Backup & Restore exp/intermediate Issue that requires some project experience kind/enhancement Enhancement, improvement, extension needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) platform/all reviewed/lgtm Has approval for merging size/m Size of pull request is medium (see gardener-robot robot/bots/size.py) status/in-progress Issue is in progress/work
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@swapnilgm @georgekuruvillak @shreyas-s-rao @gardener-robot-ci-1