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

ARROW-4479: [Plasma] Add S3 as external store for Plasma #3559

Closed
wants to merge 3 commits into from

Conversation

anuragkh
Copy link
Contributor

@anuragkh anuragkh commented Feb 5, 2019

https://issues.apache.org/jira/browse/ARROW-4479

This PR adds S3 as an external store, allowing objects to be evicted to S3 when Plasma runs out of memory capacity.

@pcmoritz I could use feedback on the preferred method for adding S3 as a dependency -- currently the CMake build simply uses find_package(AWSSDK REQUIRED ...).

@anuragkh anuragkh changed the title [Plasma] Add S3 as external store for Plasma ARROW-4479: [Plasma] Add S3 as external store for Plasma Feb 5, 2019
@wesm
Copy link
Member

wesm commented Feb 5, 2019

There are other JIRA issues about adding S3 support to the project, e.g. https://issues.apache.org/jira/browse/ARROW-453. It would be a shame to miss the opportunity to set things up properly in ThirdpartyToolchain.cmake so that other parts of the C++ project can develop against the AWS SDK

@codecov-io
Copy link

codecov-io commented Feb 5, 2019

Codecov Report

Merging #3559 into master will increase coverage by 0.75%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3559      +/-   ##
==========================================
+ Coverage   87.84%   88.59%   +0.75%     
==========================================
  Files         739      538     -201     
  Lines       90879    73567   -17312     
  Branches     1252        0    -1252     
==========================================
- Hits        79829    65175   -14654     
+ Misses      10933     8392    -2541     
+ Partials      117        0     -117
Impacted Files Coverage Δ
cpp/src/arrow/array/builder_dict.h 50% <0%> (-38.89%) ⬇️
cpp/src/gandiva/decimal_type_util.cc 68.96% <0%> (-31.04%) ⬇️
cpp/src/gandiva/precompiled/decimal_ops.cc 66.66% <0%> (-29.63%) ⬇️
...p/src/gandiva/precompiled/epoch_time_point_test.cc 72.54% <0%> (-27.46%) ⬇️
cpp/src/gandiva/basic_decimal_scalar.h 80% <0%> (-20%) ⬇️
cpp/src/gandiva/function_signature_test.cc 73.07% <0%> (-19.61%) ⬇️
cpp/src/gandiva/precompiled/time_test.cc 86.62% <0%> (-13.12%) ⬇️
python/pyarrow/compat.py 77.63% <0%> (-12.84%) ⬇️
cpp/src/gandiva/decimal_ir.cc 57.89% <0%> (-12.37%) ⬇️
cpp/src/gandiva/configuration.h 88.23% <0%> (-11.77%) ⬇️
... and 528 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a49fa50...a37d180. Read the comment docs.

@anuragkh
Copy link
Contributor Author

anuragkh commented Feb 8, 2019

@wesm @pcmoritz Sorry for the delay -- I got caught up with some other work.

I added build support for awssdk consistent with arrow's approach in ThirdpartyToolchain.cmake. Look forward to your feedback.

@xhochy
Copy link
Member

xhochy commented Feb 8, 2019

Rebased.

@wesm
Copy link
Member

wesm commented Mar 14, 2019

Sorry for the delay. This will have to be rebased after the pending CMake refactor goes in, stay tuned.

In the interest of hardening this feature I'm going to move the issue to 0.14 so we can give thoughtful consideration to S3 support throughout the project

@emkornfield
Copy link
Contributor

@anuragkh the CMake PR has been merged, do you have time to rebase?

@kszucs kszucs force-pushed the master branch 2 times, most recently from ed180da to 85fe336 Compare July 22, 2019 19:29
@emkornfield
Copy link
Contributor

Closing this PR for now. I believe we are working on S3 support as part of datasets so it might make this easier once that is done.

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.

None yet

5 participants