fix(csharp): apply CloudFetch timeout and remove redundant HttpClient#112
Merged
eric-wang-1990 merged 1 commit intomainfrom Jan 6, 2026
Merged
Conversation
277c10f to
276ccfa
Compare
jadewang-db
approved these changes
Jan 5, 2026
eric-wang-1990
added a commit
that referenced
this pull request
Jan 6, 2026
## 🥞 Stacked PR Use this [link](https://github.com/adbc-drivers/databricks/pull/111/files) to review incremental changes. - [**stack/e.wang/add-thrift-call-verification**](#111) [[Files changed](https://github.com/adbc-drivers/databricks/pull/111/files)] - [stack/e.wang/fix-cloudfetch-timeout](#112) [[Files changed](https://github.com/adbc-drivers/databricks/pull/112/files/fbd12dc00750ec405c2130138ce59f4d1db9c46d..277c10fc5ba030ae3f3f18d4026c38186fc8941f)] --------- ## What's Changed This PR adds comprehensive call verification to CloudFetch proxy tests using dynamic baseline measurement. ### Changes - **Dynamic baseline verification**: Tests now establish baselines by running queries without failure scenarios, then compare actual vs expected call counts - **CloudFetch failure scenarios**: Added verification for: - cloudfetch_expired_link: Verifies FetchResults is called again to refresh expired links - cloudfetch_403: Verifies FetchResults is called again after 403 Forbidden - cloudfetch_timeout: Verifies retry behavior with configurable timeout - cloudfetch_connection_reset: Verifies retry behavior on connection reset - **Helper methods**: Added CreateProxiedConnectionWithParameters for tests requiring custom configuration - **Test infrastructure**: Uses ProxyControlClient to count Thrift method calls and cloud downloads ### Testing The tests validate that the driver correctly: 1. Refreshes CloudFetch download links by calling FetchResults again when links expire or return 403 2. Retries CloudFetch downloads with exponential backoff on timeout or connection reset 3. Uses dynamic baselines to account for different result set sizes and prefetch behavior Note: The CloudFetchTimeout test currently fails because the Thrift driver does not apply the configured CloudFetch timeout to the HttpClient. This will be addressed in a follow-up PR. Co-authored-by: Claude Sonnet 4.5 <noreply@anthropic.com>
This commit addresses two issues in the CloudFetch implementation: 1. **Apply CloudFetch timeout configuration** - CloudFetchConfiguration.TimeoutMinutes was being read but never applied - CloudFetch downloads used the default HttpClient timeout (100s) instead of configured timeout - Fix: Apply timeout in CloudFetchDownloader constructor following OAuthClientCredentialsProvider pattern - Impact: CloudFetch downloads now respect configured timeout (default: 5 minutes) - Fixes CloudFetchTimeout test that was failing because timeouts never occurred 2. **Remove redundant HttpClient from CloudFetchDownloadManager** - CloudFetchDownloadManager received an HttpClient parameter but never used it for operations - Only used it for disposal, which is incorrect as it doesn't own the resource - The actual HTTP operations are performed by CloudFetchDownloader which has its own HttpClient - Fix: Remove httpClient parameter from CloudFetchDownloadManager constructor and field - HttpClient lifecycle is now solely managed by its actual owner Changes: - CloudFetchDownloader.cs: Apply config.TimeoutMinutes to _httpClient - CloudFetchDownloadManager.cs: Remove unused _httpClient field and parameter - CloudFetchReaderFactory.cs: Remove httpClient argument in both CreateForThrift methods 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
276ccfa to
89b8b4e
Compare
9c90ff8 to
89b8b4e
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What's Changed
This PR addresses two issues in the CloudFetch implementation and one fix in the proxy test infrastructure:
1. Apply CloudFetch Timeout Configuration
Problem: The
adbc.databricks.cloudfetch.timeout_minutesconfiguration parameter was being read but never applied to the HttpClient. The HttpClient used its default 100-second timeout instead of the configured value (default: 5 minutes).Solution: Added timeout configuration in
CloudFetchDownloaderconstructor:Impact: The CloudFetch HTTP client now correctly respects the user-configured timeout value, allowing for proper timeout behavior in CloudFetch downloads.
Files Changed:
csharp/src/Reader/CloudFetch/CloudFetchDownloader.cs2. Remove Redundant HttpClient from CloudFetchDownloadManager
Problem:
CloudFetchDownloadManageraccepted anHttpClientparameter but never used it for any operations. It only disposed of it, which is incorrect since it doesn't own the resource. The actual HTTP operations are performed byCloudFetchDownloader.Solution: Removed the redundant
httpClientparameter from:CloudFetchDownloadManagerconstructor and fieldCloudFetchReaderFactory.CreateThriftReader()callCloudFetchReaderFactory.CreateStatementExecutionReader()callImpact: Cleaner code architecture with proper resource ownership. The HttpClient is now owned solely by CloudFetchDownloader, which actually uses it for HTTP operations.
Files Changed:
csharp/src/Reader/CloudFetch/CloudFetchDownloadManager.cscsharp/src/Reader/CloudFetch/CloudFetchReaderFactory.cs3. Fix Proxy Timing for Test Reliability
Problem: The mitmproxy addon was disabling failure scenarios AFTER the delay completed, causing race conditions. When the client timed out and retried, the scenario was still enabled, triggering the delay again.
Solution: Modified
mitmproxy_addon.pyto disable the scenario immediately when triggered, before the delay starts:Impact: CloudFetch timeout tests now behave correctly - only the first request is delayed, and retry attempts succeed immediately.
Files Changed:
test-infrastructure/proxy-server/mitmproxy_addon.pyTesting
All 5 CloudFetch proxy tests now pass:
CloudFetchConnectionReset_RetriesWithExponentialBackoffCloudFetchExpiredLink_RefreshesLinkViaFetchResultsNormalCloudFetch_SucceedsWithoutFailureScenariosCloudFetchTimeout_RetriesWithExponentialBackoffCloudFetch403_RefreshesLinkViaFetchResults