HTTPClient caching across FileHandles #239
Draft
Conversation
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
carlopi
force-pushed
the
get_or_create_client_cache
branch
from
23f1960 to
eaf9ca6
Compare
carlopi
force-pushed
the
get_or_create_client_cache
branch
from
eaf9ca6 to
39c35ce
Compare
lnkuiper
reviewed
Feb 3, 2026
Collaborator
lnkuiper
left a comment
lnkuiper
left a comment
There was a problem hiding this comment.
Looks good! I have left two minor comments below, and I have a question about testing. Our current HTTP stats (and logs) currently don't show # of handshakes:
┌─────────────────────────────────────┐
│┌───────────────────────────────────┐│
││ HTTPFS HTTP Stats ││
││ ││
││ in: 131.2 MiB ││
││ out: 0 bytes ││
││ #HEAD: 1 ││
││ #GET: 9 ││
││ #PUT: 0 ││
││ #POST: 0 ││
││ #DELETE: 0 ││
│└───────────────────────────────────┘│
└─────────────────────────────────────┘
Would it be possible to measure this and add a test so we can guarantee that this continues to work?
| shared_ptr<HTTPClientCache> HTTPFileSystem::GetOrCreateClientCache(const string &path) { | ||
| lock_guard<mutex> lock(client_cache_map_lock); | ||
|
|
||
| if (auto existing = lru_client_cache.Get(path)) { |
Collaborator
There was a problem hiding this comment.
I get what you're doing here, and it looks correct, but we try to avoid assignment in if conditions because it's error-prone. Could you split this up?
| SELECT count(*) FROM duckdb_logs_parsed('HTTP') WHERE request.type = 'GET' GROUP BY request.type; | ||
| ---- | ||
| 9 | ||
| 8 |
Collaborator
There was a problem hiding this comment.
I think this change is already in a patch in the duckdb repo on v1.5-variegata. Not a problem if you add the change here too as long as CI is happy
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Currently
HTTPClientCaches are per FileHandle, after this PR they each FileHandle has ashared_ptr<HTTPClientCache>that allows to share the clients across the same base-url.An LruCache is kept to reachable
HTTPClientCache's, that allows repeated network call, even acrossFileHandle's, to avoid unnecessary network handshakes.This builds on top of LruCache made available in
duckdb/duckdbin duckdb/duckdb#20157 (and generalized in duckdb/duckdb#20757 and the infrastructure to re-initialized clients, recently touched on via #232.Ownership model is fully RAII, thanks to
shared_ptr's, note that this means that there might be still aliveHTTPClientCache, say a very long livedFileHandle, but those are not reachable anymore.This can be improved on, but for now PR to improve current status and add infrastructure.
Cache size is fixed at 256, arguably it should be configurable.
This should somewhat visibly improve performances of repeated operations to same
BaseUrl(), but apart from timing this should not bring observable differences.CI-wise, main goal here is checking whether it's a pass.