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

Port Add stream upload (multi-part upload) #2147

Merged
merged 7 commits into from
Jul 26, 2022

Conversation

alamb
Copy link
Contributor

@alamb alamb commented Jul 23, 2022

Draft until I have ported over the integration tests for the object_store crate: #2148

Which issue does this PR close?

Re #2030

Rationale for this change

Multipart upload support was added to the object store in influxdata/object_store_rs#20
by @wjones127 and @tustvold after the snapshot that was donated to the ASF in #2081.

What changes are included in this PR?

Cherry-pick code from influxdata/object_store_rs#20 to this repo

Specifically, what I did was

$ git fetch git@github.com:influxdata/object_store_rs.git
$ git checkout -b alamb/port-ulti-part-upload
$ git cherry-pick fe716c3
# manually resolved some conflicts

Are there any user-facing changes?

mutli-part uploads!

@alamb alamb added the object-store Object Store Interface label Jul 23, 2022
wjones127 and others added 2 commits July 23, 2022 14:38
* feat: Implement multi-part upload

Co-authored-by: Raphael Taylor-Davies <r.taylordavies@googlemail.com>

* chore: simplify local file implementation

* chore: Remove pin-project

* feat: make cleanup_upload() top-level

* docs: Add some docs for upload

* chore: fix linting issue

* fix: rename to put_multipart

* feat: Implement multi-part upload for GCP

* fix: Get GCS test to pass

* chore: remove more upload language

* fix: Add guard to test so we don't run with fake gcs server

* chore: small tweaks

* fix: apply suggestions from code review

Co-authored-by: Raphael Taylor-Davies <1781103+tustvold@users.noreply.github.com>

* feat: switch to quick-xml

* feat: remove throttle implementation of multipart

* fix: rename from cleanup to abort

* feat: enforce upload not readable until shutdown

* fix: ensure we close files before moving them

* chore: fix lint issue

Co-authored-by: Raphael Taylor-Davies <r.taylordavies@googlemail.com>
Co-authored-by: Raphael Taylor-Davies <1781103+tustvold@users.noreply.github.com>
@alamb alamb force-pushed the alamb/port-ulti-part-upload branch from 8d9aff7 to 5875b13 Compare July 23, 2022 19:38
@alamb alamb marked this pull request as draft July 23, 2022 19:38
@codecov-commenter
Copy link

codecov-commenter commented Jul 23, 2022

Codecov Report

Merging #2147 (905a0e7) into master (ada108b) will decrease coverage by 0.43%.
The diff coverage is 32.73%.

@@            Coverage Diff             @@
##           master    #2147      +/-   ##
==========================================
- Coverage   82.86%   82.42%   -0.44%     
==========================================
  Files         237      238       +1     
  Lines       61429    61963     +534     
==========================================
+ Hits        50902    51074     +172     
- Misses      10527    10889     +362     
Impacted Files Coverage Δ
object_store/src/aws.rs 0.00% <0.00%> (ø)
object_store/src/azure.rs 0.00% <0.00%> (ø)
object_store/src/gcp.rs 0.00% <0.00%> (ø)
object_store/src/multipart.rs 0.00% <0.00%> (ø)
object_store/src/throttle.rs 94.63% <0.00%> (-1.89%) ⬇️
object_store/src/local.rs 79.47% <72.78%> (-3.42%) ⬇️
object_store/src/memory.rs 91.79% <85.00%> (-0.39%) ⬇️
object_store/src/lib.rs 86.75% <100.00%> (+2.33%) ⬆️
parquet/src/encodings/encoding.rs 93.43% <0.00%> (-0.20%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us.

@alamb
Copy link
Contributor Author

alamb commented Jul 25, 2022

I am hopeful once the action checks run on this repo this PR will be ready for review

@alamb alamb marked this pull request as ready for review July 25, 2022 20:02
@alamb
Copy link
Contributor Author

alamb commented Jul 25, 2022

@wjones127 and @tustvold this is now ready for review

Copy link
Member

@wjones127 wjones127 left a comment

Choose a reason for hiding this comment

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

Thank you for moving this over!

@alamb alamb merged commit 19fd885 into apache:master Jul 26, 2022
@alamb alamb deleted the alamb/port-ulti-part-upload branch July 26, 2022 14:20
@ursabot
Copy link

ursabot commented Jul 26, 2022

Benchmark runs are scheduled for baseline = aeb2776 and contender = 19fd885. 19fd885 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on ec2-t3-xlarge-us-east-2] ec2-t3-xlarge-us-east-2
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on test-mac-arm] test-mac-arm
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on ursa-i9-9960x] ursa-i9-9960x
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on ursa-thinkcentre-m75q] ursa-thinkcentre-m75q
Buildkite builds:
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
object-store Object Store Interface
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants