refactor(io): hand out S3 delegates by shared_ptr and build them off the lock - #898
plusplusjiajia wants to merge 4 commits into
Conversation
27bac8f to
8f33e30
Compare
8f33e30 to
ea3d7cd
Compare
3e40be3 to
896b3ce
Compare
| std::vector<std::pair<std::string, std::unique_ptr<ArrowFileSystemFileIO>>> | ||
| file_io_by_prefix; | ||
| file_io_by_prefix.reserve(storage_credentials.size()); | ||
| ICEBERG_ASSIGN_OR_RAISE(auto delegates, BuildDelegates(storage_credentials)); |
There was a problem hiding this comment.
REST installs credentials only at FileIO creation. C++ has no refresh path. What production path reinstalls them on a live S3 FileIO?
|
|
||
| /// \brief Build a delegate for each credential this FileIO can serve. | ||
| /// | ||
| /// Lock-free on purpose: building an S3 client can reach out to discover a |
There was a problem hiding this comment.
ResolvingFileIO still builds the first S3 client under its lock. Region discovery can block other operations. Can we move it out?
There was a problem hiding this comment.
@wgtmac Good catch, thanks! Fixed: ResolvingFileIO now loads without its lock and drops delegates built from replaced credentials
| fallback = default_file_io_; | ||
| by_prefix = file_io_by_prefix_; | ||
| } | ||
| std::vector<std::pair<std::shared_ptr<ArrowFileSystemFileIO>, std::vector<std::string>>> |
There was a problem hiding this comment.
This retains the pre-existing fail-fast behavior. Java attempts all batches and counts failures. Is matching that behavior in scope?
| credential("s3://bucket-b", "key-b")}), | ||
| IsOk()); | ||
|
|
||
| auto status = result.value()->DeleteFiles({"s3://bucket-a/%ZZ.parquet", |
There was a problem hiding this comment.
The first URI fails parsing, so bucket-b is never reached. Can this test verify that both prefixes receive deletes?
There was a problem hiding this comment.
@wgtmac Thanks, fixed: the test now writes under both prefixes, deletes in one call, and checks each file is gone.
| for (int i = 0; i < 4; ++i) { | ||
| operations.emplace_back([&] { | ||
| while (!stop.load()) { | ||
| if (!result.value()->NewInputFile("s3://bucket/key").has_value()) { |
There was a problem hiding this comment.
NewInputFile only creates a wrapper. Could this test hold a file across an install and perform an actual read?
There was a problem hiding this comment.
@wgtmac Thanks, added InputFileOutlivesCredentialInstall: it reads one InputFile before and after an install.
| ArrowS3FileIO(std::shared_ptr<::arrow::fs::FileSystem> arrow_fs, | ||
| std::unordered_map<std::string, std::string> default_properties) | ||
| : default_file_io_(std::move(arrow_fs)), | ||
| : default_file_io_(std::make_shared<ArrowFileSystemFileIO>(std::move(arrow_fs))), |
There was a problem hiding this comment.
@wgtmac MatchDelegate now returns a shared_ptr, so the default delegate is one too, which keeps a single owning return type.
…ss installs ResolvingFileIO::FileIOForPath held its mutex across FileIORegistry::Load and the delegate's SetStorageCredentials, so building the first S3 client (which can look up a bucket region) stalled every other operation on the resolver. It now loads without the lock and caches the result only if no credential install happened meanwhile; otherwise it loads again with the new credentials. Delegates retired by an install are torn down outside the lock. Tests now exercise real IO where the old ones stopped short: the multi-prefix DeleteFiles test failed URI parsing before reaching any delegate, and the concurrency test only built input-file wrappers. The replacements write, delete and read through the test object store, and the stress test waits for every worker to run before installing.
896b3ce to
bc689cc
Compare
Arrow's S3FileSystem::DeleteFile issues a HEAD first and fails with PathNotFound for a missing key, so deleting a missing key does not succeed through this path. The read-back check stays; only its stated reason was wrong.
Two comments said building an S3 client can look up a bucket region. It does not here: Arrow resolves a bucket's region only in S3Options::FromUri, which this code does not use. What can block is the AWS SDK waiting on the EC2 metadata service when no static keys are set (about 4 s per build off EC2); a build from static keys does not wait.
This branch has not been deployed
2 participants
ArrowS3FileIO's per-credential delegates were raw references into an unsynchronized vector:
SetStorageCredentialsrebuilt it while a concurrent operation could still hold one, and building an S3 client — which can block on the EC2 metadata service when no static keys are set — happened with no locking at all.This gives ArrowS3FileIO the treatment #889 gave ResolvingFileIO: delegates held and handed out by
shared_ptr, state under ashared_mutex, delegate construction moved off the lock, andDeleteFilesmatching every location against one snapshot so a batch sees a single delegate generation.SupportsStorageCredentials::credentials()returns by value, since a reference can dangle once a concurrent install replaces the vector.