smaheshwar-pltr commented on code in PR #1090:
URL: https://github.com/apache/iceberg-cpp/pull/1090#discussion_r4483838281
##########
src/iceberg/catalog/rest/http_client.cc:
##########
@@ -286,34 +337,30 @@ Result<HttpResponse> HttpClient::PostForm(
AuthenticateRequest(session, HttpMethod::kPost, std::move(request_uri),
MergeHeaders(default_headers_, form_headers),
std::move(encoded_body)));
- cpr::Response response =
- cpr::Post(cpr::Url{authenticated.url}, cpr::Body{authenticated.body},
- ToCprHeader(authenticated), kNoFollowRedirects,
*connection_pool_,
- cpr::ConnectTimeout{connection_timeout_},
cpr::Timeout{socket_timeout_});
-
- ICEBERG_RETURN_UNEXPECTED(HandleFailureResponse(response, error_handler));
- HttpResponse http_response;
- http_response.impl_ =
std::make_unique<HttpResponse::Impl>(std::move(response));
- return http_response;
+ return connection_pools_->Run(
+ [&](const cpr::ConnectionPool& pool) {
+ return cpr::Post(cpr::Url{authenticated.url},
cpr::Body{authenticated.body},
+ ToCprHeader(authenticated), kNoFollowRedirects, pool,
+ cpr::ConnectTimeout{connection_timeout_},
+ cpr::Timeout{socket_timeout_});
+ },
+ error_handler);
}
Result<HttpResponse> HttpClient::Head(
const std::string& path, const std::unordered_map<std::string,
std::string>& headers,
const ErrorHandler& error_handler, auth::AuthSession& session) {
- ICEBERG_ASSIGN_OR_RAISE(auto request_uri, RequestUri(path));
Review Comment:
Ah great catch thank you, I missed this up in the rebase. I did think it was
a bit odd that nothing failed here, so I've added a regression test in
https://github.com/apache/iceberg-cpp/pull/1090/commits/1f820cc5618fcc8ea1e05dc5df3ab68e45d8092e
for the HTTP client to catch breaks like this (in this PR). Happy to remove /
separate out but figured it'd be fine to go in with this
##########
src/iceberg/catalog/rest/http_client.cc:
##########
@@ -286,34 +337,30 @@ Result<HttpResponse> HttpClient::PostForm(
AuthenticateRequest(session, HttpMethod::kPost, std::move(request_uri),
MergeHeaders(default_headers_, form_headers),
std::move(encoded_body)));
- cpr::Response response =
- cpr::Post(cpr::Url{authenticated.url}, cpr::Body{authenticated.body},
- ToCprHeader(authenticated), kNoFollowRedirects,
*connection_pool_,
- cpr::ConnectTimeout{connection_timeout_},
cpr::Timeout{socket_timeout_});
-
- ICEBERG_RETURN_UNEXPECTED(HandleFailureResponse(response, error_handler));
- HttpResponse http_response;
- http_response.impl_ =
std::make_unique<HttpResponse::Impl>(std::move(response));
- return http_response;
+ return connection_pools_->Run(
+ [&](const cpr::ConnectionPool& pool) {
+ return cpr::Post(cpr::Url{authenticated.url},
cpr::Body{authenticated.body},
+ ToCprHeader(authenticated), kNoFollowRedirects, pool,
+ cpr::ConnectTimeout{connection_timeout_},
+ cpr::Timeout{socket_timeout_});
+ },
+ error_handler);
}
Result<HttpResponse> HttpClient::Head(
const std::string& path, const std::unordered_map<std::string,
std::string>& headers,
const ErrorHandler& error_handler, auth::AuthSession& session) {
- ICEBERG_ASSIGN_OR_RAISE(auto request_uri, RequestUri(path));
Review Comment:
Ah great catch thank you, I messed this up in the rebase. I did think it was
a bit odd that nothing failed here, so I've added a regression test in
https://github.com/apache/iceberg-cpp/pull/1090/commits/1f820cc5618fcc8ea1e05dc5df3ab68e45d8092e
for the HTTP client to catch breaks like this (in this PR). Happy to remove /
separate out but figured it'd be fine to go in with this
--
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]
