Repository navigation
Avoid throttling: S3FileSystem::CreateDir should check the existence first rather than creating the parent path all the time #41493
Description
Describe the enhancement requested
Hi, I have a use case that thousands of jobs are writing hive partitioned parquet files daily to the same bucket via S3FS filesystem. Note each job may generate from single digit to a few thousand parquet files depending on the volume from its data source.
After abstraction, these jobs follow regex path patterns like s3fs://my-S3-bucket/<vendor-name>/<fruit-type>/<color>/<origination>/<creation_date>/<data-center-location>/date=YYYY-MM-DD/.... The gist here is a lot of keys are being created at the same time hense jobs hits AWS Error SLOW_DOWN. during Put Object operation: The object exceeded the rate limit for object mutation operations(create, update, and delete). Please reduce your rate request error. frequently throughout the day.
After investigation i realize they are creating too many objects in S3FileSystem::CreateDir(..) function one by one. My local experiments show that if my implementation avoids/checks the existence of the path first then call impl_->CreateEmptyDir(...) only when necessary, it addresses the issue in my production environment.
(I understand various cloud vendors have various IO limits on a single bucket, in order to completely fix the the issue is another story to my daily work)
I'm proposing a code change like below. Hi @pitrou I see you are the main author of s3fs.cc, can you pls share your insights when you have time?
Also even with a vanilla build S3FS test fails quite a few on my mac... can you guide how to make them run successfully..?
Many thanks.
diff --git a/cpp/src/arrow/filesystem/s3fs.cc b/cpp/src/arrow/filesystem/s3fs.cc
index 640888e1c..782d5f75d 100644
--- a/cpp/src/arrow/filesystem/s3fs.cc
+++ b/cpp/src/arrow/filesystem/s3fs.cc
@@ -2871,7 +2871,10 @@ Status S3FileSystem::CreateDir(const std::string& s, bool recursive) {
for (const auto& part : path.key_parts) {
parent_key += part;
parent_key += kSep;
- RETURN_NOT_OK(impl_->CreateEmptyDir(path.bucket, parent_key));
+ ARROW_ASSIGN_OR_RAISE(FileInfo parent_key_info, this->GetFileInfo(parent_key));
+ if (parent_key_info.type() == FileType::NotFound) {
+ RETURN_NOT_OK(impl_->CreateEmptyDir(path.bucket, parent_key));
+ }
}
return Status::OK();
} else {
TestS3FS.CreateDir even fails with a Clean build :sigh
➜ build ninja && ./debug/arrow-s3fs-test --gtest_filter="TestS3FS.CreateDir"
ninja: no work to do.
Running main() from /Users/haochengliu/Documents/projects/Arrow/build/_deps/googletest-src/googletest/src/gtest_main.cc
Note: Google Test filter = TestS3FS.CreateDir
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from TestS3FS
[ RUN ] TestS3FS.CreateDir
/Users/haochengliu/Documents/projects/Arrow/arrow/cpp/src/arrow/filesystem/s3fs_test.cc:934: Failure
Failed
Expected 'fs_->CreateDir("bucket/somefile")' to fail with IOError, but got OK
[ FAILED ] TestS3FS.CreateDir (219 ms)
[----------] 1 test from TestS3FS (219 ms total)
Component(s)
C++
Activity
MattShrider commentedon May 21, 2024
We're running into a similar issue in R.
arrow::write_dataset(data, path = s3bucket$path('/foo/bar/baz'))attempts to create/fooand/foo/bareven if those directories already exist. In our setup we only allow writing directly into/baror/baz, so we get an unnecessary permission error when it tries to write/foo.HaochengLIU commentedon May 22, 2024
Hi @westonpace @pitrou when you have time can you guide how to make S3FS tests pass in a vanilla test build? Once I'm unblocked happy to propose a fix and ask for review.
westonpace commentedon May 22, 2024
Hmm...I think these tests use minio? Have you setup minio? I wonder if the issue might have something to do with minio version?
HaochengLIU commentedon May 23, 2024
Hi Weston!
I just used brew to install
minio(stable 20240510014138 (bottled), HEAD) and launched it viabrew services start miniobut same deal with the above test failure... Humm, any other ideas?My personal laptop is a mac, one thing I can try is to use my work linux host with that test tomorrow
HaochengLIU commentedon May 23, 2024
Another thing i could do this weekend is to create the PR first and see how it goes on github
pitrou commentedon May 23, 2024
Yes, and you can also use docker to easily reproduce the CI configurations:
https://arrow.apache.org/docs/developers/continuous_integration/docker.html
You can do that as well, though iterating will be more painful for you than if you can recreate the test setup locally.
pitrou commentedon May 23, 2024
As to the issue itself: the reason
CreateDirwas implemented like this is that we're assuming that, usually, the directory doesn't exist already. So it's an optimization to try directory creation and catch the error, rather than issue two dependent IOs.If, OTOH, there are situations where the directory exists most of the time, then this becomes a pessimization (at least for S3 with its accounting rules).
Perhaps
CreateDircan take an optional argument, or perhaps this can be an extra option insideS3Options.HaochengLIU commentedon May 24, 2024
I tried few things and here is my latest finding:
Both "passes" when minio is not installed
Both failed with the presence of minio...
Which makes me wonder if minio is ever used in Arrow testing pipelines?
Anyway, I will try to create a PR with an extra options inside
S3Optionsvery soonGH-41493: [C++][S3] Add a new option to check existence before Create…
pitrou commentedon Jun 4, 2024
Issue resolved by pull request 41822
#41822
GH-41960: Expose new S3 option check_directory_existence_before_creat…