You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "andygrove (via GitHub)" <gi...@apache.org> on 2026/03/26 20:39:23 UTC

[PR] fix: cache object stores and bucket regions to reduce DNS query volume [datafusion-comet]

andygrove opened a new pull request, #3802:
URL: https://github.com/apache/datafusion-comet/pull/3802

   ## Which issue does this PR close?
   
   <!--Closes #.-->
   
   ## Rationale for this change
   
   When reading Parquet files from S3, each call to `initRecordBatchReader` creates a new `SessionContext`, `RuntimeEnv`, and S3 `ObjectStore` client. Each `ObjectStore` instance creates a new `reqwest` HTTP client with its own connection pool, requiring fresh DNS resolution for every file opened.
   
   In TPC-DS-scale workloads with thousands of Parquet files, this generates excessive DNS queries that can overwhelm DNS resolvers (e.g., Route53 Resolver limits in EKS environments), causing `UnknownHostException` errors, intermittent S3 connectivity failures, and job failures under high concurrency.
   
   Native Spark does not have this problem because Hadoop's `S3AFileSystem` maintains singleton instances per filesystem URI (via `FileSystem.get()` cache) and benefits from JVM-level DNS caching. Comet's native Rust layer bypasses all of this.
   
   Additionally, when no region is explicitly configured, each file read triggers a `HeadBucket` API call with a throwaway `reqwest::Client` to auto-detect the bucket region, doubling the DNS query volume.
   
   ## What changes are included in this PR?
   
   Two caches are introduced in the native Rust layer:
   
   1. **ObjectStore cache** (`parquet_support.rs`): A global cache keyed by `(URL prefix, config hash)` in `prepare_object_store_with_configs()`. Subsequent reads from the same bucket with the same configuration reuse the existing `ObjectStore` instance, enabling HTTP connection pooling via `reqwest` and eliminating redundant DNS lookups.
   
   2. **Region resolution cache** (`s3.rs`): A per-bucket cache in `resolve_bucket_region()` that ensures the `HeadBucket` API call (used to auto-detect bucket region when not explicitly configured) happens only once per bucket per JVM lifetime.
   
   Together these changes reduce DNS query volume from O(num_files) to O(num_distinct_buckets) per executor.
   
   **Workaround for users on current versions:** Set `fs.s3a.endpoint.region` explicitly to avoid the `HeadBucket` auto-detection entirely.
   
   ## How are these changes tested?
   
   - Existing unit tests in `parquet_support.rs` continue to pass (they test local filesystem paths which exercise the same `prepare_object_store_with_configs` code path).
   - Clippy passes with no warnings.
   - The caching is transparent — it returns the same `ObjectStore` instances that would have been created without caching, so existing S3 integration tests cover correctness.
   - Manual validation recommended in an EKS environment with S3-backed TPC-DS workloads to confirm DNS query volume reduction.


-- 
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: github-unsubscribe@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org


Re: [PR] fix: cache object stores and bucket regions to reduce DNS query volume [datafusion-comet]

Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #3802:
URL: https://github.com/apache/datafusion-comet/pull/3802#discussion_r3010505756


##########
native/core/src/parquet/parquet_support.rs:
##########
@@ -444,6 +447,32 @@ fn create_hdfs_object_store(
     })
 }
 
+type ObjectStoreCache = RwLock<HashMap<(String, u64), Arc<dyn ObjectStore>>>;
+
+/// Global cache of object stores keyed by (url_key, config_hash).
+///
+/// This avoids creating a new S3 client (and thus a new HTTP connection pool and DNS
+/// resolution) for every Parquet file read. Without this cache, each call to
+/// `initRecordBatchReader` from the JVM creates a fresh `reqwest::Client`, leading to
+/// excessive DNS queries that can overwhelm DNS resolvers (e.g., Route53 Resolver limits
+/// in EKS environments).
+fn object_store_cache() -> &'static ObjectStoreCache {
+    static CACHE: OnceLock<ObjectStoreCache> = OnceLock::new();

Review Comment:
   @mbutrovich could you take another look?



-- 
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: github-unsubscribe@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org


Re: [PR] fix: cache object stores and bucket regions to reduce DNS query volume [datafusion-comet]

Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on code in PR #3802:
URL: https://github.com/apache/datafusion-comet/pull/3802#discussion_r3060602315


##########
native/core/src/parquet/parquet_support.rs:
##########
@@ -467,17 +520,45 @@ pub(crate) fn prepare_object_store_with_configs(
         &url[url::Position::BeforeHost..url::Position::AfterPort],
     );
 
-    let (object_store, object_store_path): (Box<dyn ObjectStore>, Path) = if is_hdfs_scheme {
-        create_hdfs_object_store(&url)
-    } else if scheme == "s3" {
-        objectstore::s3::create_store(&url, object_store_configs, Duration::from_secs(300))
-    } else {
-        parse_url(&url)
-    }
-    .map_err(|e| ExecutionError::GeneralError(e.to_string()))?;
+    let config_hash = hash_object_store_configs(object_store_configs);
+    let cache_key = (url_key.clone(), config_hash);
+
+    // Check the cache first to reuse existing object store instances.
+    // This enables HTTP connection pooling and avoids redundant DNS lookups.
+    let cached = {
+        let cache = object_store_cache()
+            .read()
+            .map_err(|e| ExecutionError::GeneralError(format!("Object store cache error: {e}")))?;
+        cache.get(&cache_key).cloned()
+    };
+
+    let (object_store, object_store_path): (Arc<dyn ObjectStore>, Path) =
+        if let Some(store) = cached {
+            debug!("Reusing cached object store for {url_key}");
+            let path = Path::from_url_path(url.path())
+                .map_err(|e| ExecutionError::GeneralError(e.to_string()))?;
+            (store, path)
+        } else {
+            debug!("Creating new object store for {url_key}");
+            let (store, path): (Box<dyn ObjectStore>, Path) = if is_hdfs_scheme {
+                create_hdfs_object_store(&url)
+            } else if scheme == "s3" {
+                objectstore::s3::create_store(&url, object_store_configs, Duration::from_secs(300))
+            } else {
+                parse_url(&url)
+            }
+            .map_err(|e| ExecutionError::GeneralError(e.to_string()))?;
+
+            let store: Arc<dyn ObjectStore> = Arc::from(store);
+            // Insert into cache
+            if let Ok(mut cache) = object_store_cache().write() {

Review Comment:
   why the `write()` is here?



-- 
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: github-unsubscribe@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org


Re: [PR] fix: cache object stores and bucket regions to reduce DNS query volume [datafusion-comet]

Posted by "parthchandra (via GitHub)" <gi...@apache.org>.
parthchandra commented on PR #3802:
URL: https://github.com/apache/datafusion-comet/pull/3802#issuecomment-4138706519

   How would we verify that the number of DNS requests has gone down?  
   (Note: On EKS we can get CoreDNS metrics https://docs.aws.amazon.com/eks/latest/userguide/coredns-metrics.html)


-- 
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: github-unsubscribe@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org


Re: [PR] fix: cache object stores and bucket regions to reduce DNS query volume [datafusion-comet]

Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on PR #3802:
URL: https://github.com/apache/datafusion-comet/pull/3802#issuecomment-4164585139

   > How would we verify that the number of DNS requests has gone down? (Note: On EKS we can get CoreDNS metrics https://docs.aws.amazon.com/eks/latest/userguide/coredns-metrics.html)
   
   I did confirm that this PR resolved the intermittent S3 errors that I was seeing - https://github.com/apache/datafusion-comet/pull/3802#issuecomment-4142965093
   


-- 
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: github-unsubscribe@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org


Re: [PR] fix: cache object stores and bucket regions to reduce DNS query volume [datafusion-comet]

Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove merged PR #3802:
URL: https://github.com/apache/datafusion-comet/pull/3802


-- 
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: github-unsubscribe@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org


Re: [PR] fix: cache object stores and bucket regions to reduce DNS query volume [datafusion-comet]

Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #3802:
URL: https://github.com/apache/datafusion-comet/pull/3802#discussion_r3001298235


##########
native/core/src/parquet/parquet_support.rs:
##########
@@ -444,6 +447,32 @@ fn create_hdfs_object_store(
     })
 }
 
+type ObjectStoreCache = RwLock<HashMap<(String, u64), Arc<dyn ObjectStore>>>;
+
+/// Global cache of object stores keyed by (url_key, config_hash).
+///
+/// This avoids creating a new S3 client (and thus a new HTTP connection pool and DNS
+/// resolution) for every Parquet file read. Without this cache, each call to
+/// `initRecordBatchReader` from the JVM creates a fresh `reqwest::Client`, leading to
+/// excessive DNS queries that can overwhelm DNS resolvers (e.g., Route53 Resolver limits
+/// in EKS environments).
+fn object_store_cache() -> &'static ObjectStoreCache {
+    static CACHE: OnceLock<ObjectStoreCache> = OnceLock::new();

Review Comment:
   Added docs, but I have not yet confirmed that the part about credential provider interaction is actually correct, so moved to draft for now.



-- 
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: github-unsubscribe@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org


Re: [PR] fix: cache object stores and bucket regions to reduce DNS query volume [datafusion-comet]

Posted by "mbutrovich (via GitHub)" <gi...@apache.org>.
mbutrovich commented on PR #3802:
URL: https://github.com/apache/datafusion-comet/pull/3802#issuecomment-4138082973

   Seems similar to the challenges I'm seeing in iceberg-rust https://github.com/apache/iceberg-rust/pull/2177 https://github.com/apache/iceberg-rust/issues/2172.


-- 
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: github-unsubscribe@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org


Re: [PR] fix: cache object stores and bucket regions to reduce DNS query volume [datafusion-comet]

Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on code in PR #3802:
URL: https://github.com/apache/datafusion-comet/pull/3802#discussion_r3060620657


##########
native/core/src/parquet/parquet_support.rs:
##########
@@ -467,17 +520,45 @@ pub(crate) fn prepare_object_store_with_configs(
         &url[url::Position::BeforeHost..url::Position::AfterPort],
     );
 
-    let (object_store, object_store_path): (Box<dyn ObjectStore>, Path) = if is_hdfs_scheme {
-        create_hdfs_object_store(&url)
-    } else if scheme == "s3" {
-        objectstore::s3::create_store(&url, object_store_configs, Duration::from_secs(300))
-    } else {
-        parse_url(&url)
-    }
-    .map_err(|e| ExecutionError::GeneralError(e.to_string()))?;
+    let config_hash = hash_object_store_configs(object_store_configs);
+    let cache_key = (url_key.clone(), config_hash);
+
+    // Check the cache first to reuse existing object store instances.
+    // This enables HTTP connection pooling and avoids redundant DNS lookups.
+    let cached = {
+        let cache = object_store_cache()
+            .read()
+            .map_err(|e| ExecutionError::GeneralError(format!("Object store cache error: {e}")))?;
+        cache.get(&cache_key).cloned()
+    };
+
+    let (object_store, object_store_path): (Arc<dyn ObjectStore>, Path) =
+        if let Some(store) = cached {
+            debug!("Reusing cached object store for {url_key}");
+            let path = Path::from_url_path(url.path())
+                .map_err(|e| ExecutionError::GeneralError(e.to_string()))?;
+            (store, path)
+        } else {
+            debug!("Creating new object store for {url_key}");
+            let (store, path): (Box<dyn ObjectStore>, Path) = if is_hdfs_scheme {
+                create_hdfs_object_store(&url)
+            } else if scheme == "s3" {
+                objectstore::s3::create_store(&url, object_store_configs, Duration::from_secs(300))
+            } else {
+                parse_url(&url)
+            }
+            .map_err(|e| ExecutionError::GeneralError(e.to_string()))?;
+
+            let store: Arc<dyn ObjectStore> = Arc::from(store);
+            // Insert into cache
+            if let Ok(mut cache) = object_store_cache().write() {

Review Comment:
   nvm, it is cache write, not a object store write



-- 
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: github-unsubscribe@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org


Re: [PR] fix: cache object stores and bucket regions to reduce DNS query volume [datafusion-comet]

Posted by "mbutrovich (via GitHub)" <gi...@apache.org>.
mbutrovich commented on code in PR #3802:
URL: https://github.com/apache/datafusion-comet/pull/3802#discussion_r3001172217


##########
native/core/src/parquet/parquet_support.rs:
##########
@@ -444,6 +447,32 @@ fn create_hdfs_object_store(
     })
 }
 
+type ObjectStoreCache = RwLock<HashMap<(String, u64), Arc<dyn ObjectStore>>>;
+
+/// Global cache of object stores keyed by (url_key, config_hash).
+///
+/// This avoids creating a new S3 client (and thus a new HTTP connection pool and DNS
+/// resolution) for every Parquet file read. Without this cache, each call to
+/// `initRecordBatchReader` from the JVM creates a fresh `reqwest::Client`, leading to
+/// excessive DNS queries that can overwhelm DNS resolvers (e.g., Route53 Resolver limits
+/// in EKS environments).
+fn object_store_cache() -> &'static ObjectStoreCache {
+    static CACHE: OnceLock<ObjectStoreCache> = OnceLock::new();

Review Comment:
   Anything `static` (global singleton) should be documented why `static` is the reasonable life cycle. In particular, I wonder about the unbounded size of a `static` cache, invalidation scenarios (what if a job runs long enough and needs new credentials passed into the object_store?), and why there was no other location with a reasonable life cycle to own this cache.



-- 
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: github-unsubscribe@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org


Re: [PR] fix: cache object stores and bucket regions to reduce DNS query volume [datafusion-comet]

Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on PR #3802:
URL: https://github.com/apache/datafusion-comet/pull/3802#issuecomment-4142965093

   I tested with TPC-H @ 1TB in AWS and this does appear to have resolved the `Generic S3 error: an error occurred while loading credentials` error that I was previously seeing


-- 
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: github-unsubscribe@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org