Replace HTTPlib client with Curl #58
Conversation
…t overwritten on the stack
| auto header_collector = make_uniq<HeaderCollector>(); | ||
| { | ||
| // Set URL | ||
| curl_easy_setopt(*curl, CURLOPT_URL, request_info->url.c_str()); |
There was a problem hiding this comment.
There's now quite a bit of repetition in these curl configurations between the different request methods. Could we perhaps move the shared config between them in a function?
| mode skip | ||
|
|
||
| query IIIIII rowsort | ||
| SELECT * from read_csv_auto('https://github.com/duckdb/duckdb/raw/9cf66f950dde0173e1a863a7659b3ecf11bf3978/data/csv/customer.csv'); |
There was a problem hiding this comment.
Maybe its nice to use logging to ensure we get the expected http headers?
for example:
pragma enable_logging('HTTP');
-- do query
select response.headers['Connection'] from duckdb_logs_parsed('HTTP');
We could verify for example that the __RESPONSE_STATUS__ header is properly parsed into the response headers: select response.headers['__RESPONSE_STATUS__'] from duckdb_logs_parsed('HTTP');
extension/httpfs/httpfs_client.cpp
Outdated
| } | ||
|
|
||
| // If header starts with HTTP/... curl has followed a redirect and we have a new Header, | ||
| // so we clear all of the current header_collection |
There was a problem hiding this comment.
not sure I understand this, the comment suggests this is going to clear the header_collection, but then doesn't?
|
Just a concern of mine, to keep in mind: could we have a branch (that I assumed was That would mean either creating a separate |
There is already a v1.3-ossivalis branch https://github.com/duckdb/duckdb-httpfs/tree/v1.3-ossivalis |
|
Maybe we should sync in a bit, v1.3-ossivalis branch is not in sync with https://github.com/duckdb/duckdb/blob/v1.3-ossivalis/.github/config/out_of_tree_extensions.cmake, and I assume it's not compatible with v1.3-ossivalis branch of duckdb/duckdb. Happy to have |
First steps to use Curl as the HTTP client. There are still some questions around when to call
curl_global_cleanup(), since it should only be called once curl is completely done.I also might not be initializing options correctly, so looking for feedback on that.