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?;
```
I could also rename `response` to `deserialized_response` to make it super
obvious what is happening.
---
Edit: After thinking a bit more about this, I'm trying to figure out what
the signature of the custom handler should be. If it is not an asynchronous
`FnOnce(Response) -> Result<R>`, then should it be a `FnOnce(Response) ->
Result<Response>`? So the handler takes in the HTTP response and checks if
there are any problems with it. If there are problems, then it returns `Err`.
But if it can't detect any problems, it returns the entire `Response` even
though it is still possible that the deserialization goes wrong? I think this
could be more confusing, but I can see points on both sides.
I should also mention that the `deserialize_unexpected_catalog_error` should
also be deserializing every response it gets into a proper error message
instead of a blanket "Received unexpected response", but I felt that was out of
scope for this PR.
--
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]
