plusplusjiajia commented on code in PR #898:
URL: https://github.com/apache/iceberg-cpp/pull/898#discussion_r4110388045
##########
src/iceberg/arrow/s3/arrow_s3_file_io.cc:
##########
@@ -209,27 +212,67 @@ class ArrowS3FileIO final : public FileIO, public
SupportsStorageCredentials {
Status SetStorageCredentials(
const std::vector<StorageCredential>& storage_credentials) override;
- const std::vector<StorageCredential>& credentials() const override {
+ std::vector<StorageCredential> credentials() const override {
+ std::shared_lock lock(mutex_);
return storage_credentials_;
}
SupportsStorageCredentials* AsSupportsStorageCredentials() override { return
this; }
private:
- ArrowFileSystemFileIO& FileIOForPath(std::string_view location);
-
- ArrowFileSystemFileIO default_file_io_;
+ /// \brief Delegate serving `location`, pinned by the caller against a
+ /// concurrent credential install.
+ std::shared_ptr<ArrowFileSystemFileIO> FileIOForPath(std::string_view
location);
+
+ using DelegatesByPrefix =
+ std::vector<std::pair<std::string,
std::shared_ptr<ArrowFileSystemFileIO>>>;
+
+ /// \brief Longest-prefix match against one consistent view of the delegates.
+ static std::shared_ptr<ArrowFileSystemFileIO> MatchDelegate(
+ const std::shared_ptr<ArrowFileSystemFileIO>& fallback,
+ const DelegatesByPrefix& by_prefix, std::string_view location);
+
+ /// \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
+ /// bucket region, which would stall every concurrent operation. Reads no
+ /// mutable member state.
+ Result<DelegatesByPrefix> BuildDelegates(
+ const std::vector<StorageCredential>& storage_credentials) const;
+
+ /// \brief Swap in credentials and delegates, handing back the retired ones.
+ ///
+ /// Callers must hold `mutex_` exclusively and let the returned generation
+ /// destruct only after releasing it: tearing down an S3 client can block on
+ /// in-flight requests, which would stall every operation.
+ void InstallCredentials(std::vector<StorageCredential>& storage_credentials,
+ DelegatesByPrefix& delegates);
+
+ std::shared_ptr<ArrowFileSystemFileIO> default_file_io_;
std::unordered_map<std::string, std::string> default_properties_;
+ // Guards everything below; shared because reads happen per file operation.
+ mutable std::shared_mutex mutex_;
std::vector<StorageCredential> storage_credentials_;
- std::vector<std::pair<std::string, std::unique_ptr<ArrowFileSystemFileIO>>>
- file_io_by_prefix_;
+ DelegatesByPrefix file_io_by_prefix_;
};
Status ArrowS3FileIO::SetStorageCredentials(
const std::vector<StorageCredential>& storage_credentials) {
- 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));
Review Comment:
@wgtmac Thanks! None yet. #892 (with #899's refresher) replaces credentials
on a live S3 FileIO; this PR makes that safe for in-flight operations.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
