connortsui20 commented on code in PR #965:
URL: https://github.com/apache/iceberg-rust/pull/965#discussion_r1957528321
##########
crates/catalog/rest/src/catalog.rs:
##########
@@ -670,21 +719,47 @@ impl Catalog for RestCatalog {
})
.build()?;
- let resp = self
- .context()
- .await?
- .client
- .query::<CommitTableResponse, ErrorResponse>(request)
- .await?;
+ let handler = |response: Response| async move {
+ if response.status() == StatusCode::OK {
+ return
deserialize_catalog_response::<CommitTableResponse>(response).await;
+ }
+
+ let error_message = match response.status() {
+ StatusCode::NOT_FOUND => "Tried to update a table that does
not exist",
+ StatusCode::CONFLICT => {
+ "CommitFailedException, one or more requirements failed.
The client may retry."
+ }
+ StatusCode::INTERNAL_SERVER_ERROR => {
+ "An unknown server-side problem occurred; the commit state
is unknown."
+ }
+ StatusCode::BAD_GATEWAY => {
+ "A gateway or proxy received an invalid response from the
upstream server; the commit state is unknown."
+ }
+ StatusCode::GATEWAY_TIMEOUT => {
+ "A server-side gateway timeout occurred; the commit state
is unknown."
+ }
+ _ => {
+ return Err(deserialize_unexpected_catalog_error(
+ response,
+ )
+ .await);
+ }
+ };
+
+ Err(Error::new(ErrorKind::Unexpected, error_message))
+ };
+
+ let response = context.client.query_catalog(request, handler).await?;
Review Comment:
> How about taking a step back and simply returning an HTTP response in
those functions? That way, we have full control over handling the HTTP response
here. We only have a limited number of APIs to implement, and they won’t grow
quickly. I’m fine with just writing them directly instead of adding generics or
callbacks.
I can try this, but I'm not sure this would make things cleaner. It just
means that you are moving where you call the serialize function from inside the
handler to outside the handler, and now then now you have to handle the error
cases outside the helper every single time. Maybe as a best of both worlds I
can specify the exact type of `response` every time? Like:
```rust
let response: ListNamespaceResponse = context.client.query_catalog(request,
handler).await?;
```
--
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]
