ARROW-453: [C++] Filesystem implementation for Amazon S3 #5167

pitrou · 2019-08-22T14:49:23Z

Unit testing is done using the Minio standalone S3 server.

Currently missing from this PR:

fix the bundled building (currently it links against the wrong version of libcrypto, ending up loading two different OpenSSL versions at runtime)

pitrou · 2019-08-22T15:18:56Z

s3fs test passed on Travis-CI: https://travis-ci.org/pitrou/arrow/jobs/575384780#L5243

pitrou · 2019-08-22T16:59:35Z

s3fs test passed on AppVeyor: https://ci.appveyor.com/project/pitrou/arrow/build/job/o639sped92wndo5f#L1740

codecov-io · 2019-08-22T23:02:24Z

Codecov Report

Merging #5167 into master will increase coverage by 1.21%.
The diff coverage is 94.24%.

@@            Coverage Diff             @@
##           master    #5167      +/-   ##
==========================================
+ Coverage   87.53%   88.75%   +1.21%     
==========================================
  Files         923      938      +15     
  Lines      140358   122444   -17914     
  Branches        0     1437    +1437     
==========================================
- Hits       122868   108679   -14189     
+ Misses      17138    13403    -3735     
- Partials      352      362      +10

Impacted Files	Coverage Δ
cpp/src/arrow/testing/util.h	`91.3% <ø> (ø)`	⬆️
cpp/src/arrow/filesystem/filesystem.h	`100% <ø> (ø)`	⬆️
cpp/src/arrow/filesystem/filesystem.cc	`80% <0%> (-6.8%)`	⬇️
cpp/src/arrow/filesystem/test_util.cc	`99.4% <100%> (+0.05%)`	⬆️
cpp/src/arrow/filesystem/filesystem_test.cc	`100% <100%> (ø)`	⬆️
cpp/src/arrow/filesystem/path_util.cc	`100% <100%> (ø)`	⬆️
cpp/src/arrow/filesystem/path_util.h	`100% <100%> (ø)`	⬆️
cpp/src/arrow/filesystem/test_util.h	`100% <100%> (ø)`	⬆️
cpp/src/arrow/filesystem/s3_internal.h	`100% <100%> (ø)`
cpp/src/arrow/filesystem/s3fs.h	`100% <100%> (ø)`
... and 207 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 17a0709...de1795a. Read the comment docs.

wesm · 2019-08-22T23:32:05Z

Will endeavor to get a review to you in the next couple working days

wesm

Thanks for getting this off the ground! S3 is a fairly complex beast. I left some sparse comments. I didn't scrutinize the specifics of the unit tests in too great detail

Suffice to say a next stage of this project will be making sure we can the test suite against "the real thing" (aka Amazon's S3), since that's how most people will be using this software in practice. We should probably also do some basic in-EC2 performance evaluations to get a baseline understanding of in-data-center and outside-of-datacenter performance in different scenarios. Maybe we can start a benchmark suite that can be modestly configured (probably using a JSON configuration file of some kind). Anyway plenty of follow up JIRA issues that can be created

.travis.yml

cpp/cmake_modules/DefineOptions.cmake

wesm · 2019-08-23T20:13:33Z

cpp/cmake_modules/ThirdpartyToolchain.cmake

+
+macro(build_awssdk)
+  message(
+    FATAL_ERROR "FIXME: Building AWS C++ SDK from source will link with wrong libcrypto")


That's super fun.

cpp/src/arrow/filesystem/s3_internal.h

wesm · 2019-08-23T20:27:20Z

cpp/src/arrow/filesystem/s3fs.h

+
+struct ARROW_EXPORT S3Options {
+  // AWS region to connect to
+  std::string region = "us-east-1";


Do you know how boto/boto3, Python s3fs, or other S3 libraries handle setting the region? Do they automatically determine the region for a bucket?

I have no idea. But it seems to be configuration-based.

wesm · 2019-08-23T22:09:36Z

cpp/src/arrow/filesystem/s3fs.cc

+
+  if (path.key.empty()) {
+    // Create bucket
+    return impl_->CreateBucket(path.bucket);


It seems like we should not make it too easy to create buckets, but in AWS access controls, creating buckets may be disallowed (so this would simply fail). I don't think there's necessarily anything to change here

wesm · 2019-08-23T22:11:04Z

cpp/src/arrow/filesystem/s3fs.cc

+    }
+
+    // XXX Should we check that no non-directory entry exists?
+    // Minio does it for us, not sure about other S3 implementations.


Definitely checking against Amazon S3 would be a good idea

wesm · 2019-08-23T22:13:39Z

cpp/src/arrow/filesystem/s3fs.cc

+    S3Model::DeleteBucketRequest req;
+    req.SetBucket(ToAwsString(path.bucket));
+    return OutcomeToStatus(impl_->client_->DeleteBucket(req));
+  }


Unclear if we want to allow this function to delete buckets (since these are the "heaviest" of S3 objects) by default. It might be better to error when trying to delete a bucket and instead offset a DeleteBucket API

At this point, the bucket is already empty, though...

cpp/src/arrow/filesystem/s3fs.cc

wesm · 2019-08-23T22:28:14Z

cpp/src/arrow/filesystem/s3fs_test.cc

+
+#define ARROW_AWS_ASSIGN_OR_FAIL(lhs, rexpr) \
+  ARROW_AWS_ASSIGN_OR_FAIL_IMPL(             \
+      ARROW_AWS_ASSIGN_OR_FAIL_NAME(_aws_error_or_value, __COUNTER__), lhs, rexpr);


Is this the same as what's in s3_internal.h?

No, this one fails in the GTest sense.

pitrou · 2019-08-26T15:42:57Z

Ok, I added a narrative test and checked it against Amazon. Also addressed some review comments.

cpp/src/arrow/filesystem/filesystem.cc

cpp/src/arrow/filesystem/s3fs.cc

fsaintjacques · 2019-08-26T14:33:35Z

cpp/src/arrow/filesystem/s3fs.cc

+
+    if (current_part_size_ >= part_upload_threshold_) {
+      // Current part large enough, upload it
+      RETURN_NOT_OK(CommitCurrentPart());


In this specific case, can't we always re-use the same buffer and adjust capacity, and use the size as indicator?

cpp/src/arrow/filesystem/s3fs.cc

fsaintjacques · 2019-08-26T15:19:48Z

cpp/src/arrow/filesystem/s3fs.h

+  std::string scheme = "https";
+
+  std::string access_key;
+  std::string secret_key;


I think we should align with what usual aws cli provide via AWSCredentialsProvider and AWSCredentialsProviderChain. I'm not sure how we should expose this in the option struct.

To add more, I suspect that users will expect that the usual mecanism works, env var, file backed config and then explicit auth creds.

cpp/src/arrow/filesystem/s3fs_test.cc

Unit testing is done using the Minio standalone S3 server.

pitrou · 2019-08-29T14:36:38Z

Thanks @fsaintjacques :-)

pitrou changed the title ~~ARROW-453: [C++] Filesystem implementation for S3~~ [WIP] ARROW-453: [C++] Filesystem implementation for S3 Aug 22, 2019

pitrou force-pushed the ARROW-453-s3fs branch from e517ddd to 2c28dce Compare August 22, 2019 14:59

pitrou force-pushed the ARROW-453-s3fs branch 3 times, most recently from adba89c to 2392160 Compare August 22, 2019 16:27

pitrou force-pushed the ARROW-453-s3fs branch from 2392160 to a0d5f28 Compare August 22, 2019 16:59

wesm changed the title ~~[WIP] ARROW-453: [C++] Filesystem implementation for S3~~ [WIP] ARROW-453: [C++] Filesystem implementation for Amazon S3 Aug 22, 2019

wesm reviewed Aug 23, 2019

View reviewed changes

pitrou force-pushed the ARROW-453-s3fs branch 2 times, most recently from 0b4a7ec to ca8c7d6 Compare August 26, 2019 15:40

pitrou changed the title ~~[WIP] ARROW-453: [C++] Filesystem implementation for Amazon S3~~ ARROW-453: [C++] Filesystem implementation for Amazon S3 Aug 26, 2019

fsaintjacques reviewed Aug 26, 2019

View reviewed changes

pitrou force-pushed the ARROW-453-s3fs branch 2 times, most recently from 9303131 to 9c1abaa Compare August 26, 2019 17:46

ARROW-453: [C++] Filesystem implementation for S3

de1795a

Unit testing is done using the Minio standalone S3 server.

pitrou force-pushed the ARROW-453-s3fs branch from 9c1abaa to de1795a Compare August 27, 2019 10:35

fsaintjacques approved these changes Aug 29, 2019

View reviewed changes

pitrou closed this in 7ec1731 Aug 29, 2019

pitrou deleted the ARROW-453-s3fs branch August 29, 2019 14:36

kkraus14 mentioned this pull request Oct 8, 2019

[FEA/Discussion] Refactor writers to use fsspec rapidsai/cudf#2760

Closed

asfimport mentioned this pull request Nov 20, 2019

[C++] Add filesystem implementation for Amazon S3 #16098

Closed

dacort mentioned this pull request Jun 23, 2023

[C++] Add option to not create parent directory with S3 delete_file #36275

Open

ARROW-453: [C++] Filesystem implementation for Amazon S3 #5167

ARROW-453: [C++] Filesystem implementation for Amazon S3 #5167

Uh oh!

Conversation

pitrou commented Aug 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pitrou commented Aug 22, 2019

Uh oh!

pitrou commented Aug 22, 2019

Uh oh!

codecov-io commented Aug 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

wesm commented Aug 22, 2019

Uh oh!

wesm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pitrou commented Aug 26, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fsaintjacques Aug 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

pitrou commented Aug 29, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

pitrou commented Aug 22, 2019 •

edited

Loading

codecov-io commented Aug 22, 2019 •

edited

Loading

fsaintjacques Aug 26, 2019 •

edited

Loading