Xuanwo commented on code in PR #965:
URL: https://github.com/apache/iceberg-rust/pull/965#discussion_r1957506030
##########
crates/catalog/rest/src/catalog.rs:
##########
@@ -312,90 +317,107 @@ impl RestCatalog {
}
}
+/// All requests and expected responses are derived from the REST catalog API
spec:
+///
https://github.com/apache/iceberg/blob/main/open-api/rest-catalog-open-api.yaml
#[async_trait]
impl Catalog for RestCatalog {
- /// List namespaces from table.
async fn list_namespaces(
&self,
parent: Option<&NamespaceIdent>,
) -> Result<Vec<NamespaceIdent>> {
- let mut request = self.context().await?.client.request(
- Method::GET,
- self.context().await?.config.namespaces_endpoint(),
- );
- if let Some(ns) = parent {
- request = request.query(&[("parent", ns.to_url_string())]);
- }
+ let context = self.context().await?;
- let resp = self
- .context()
- .await?
+ let mut request_builder = context
.client
- .query::<ListNamespaceResponse, ErrorResponse>(request.build()?)
- .await?;
+ .request(Method::GET, context.config.namespaces_endpoint());
+ // Filter on `parent={namespace}` if a parent namespace exists.
+ if let Some(namespace) = parent {
+ request_builder = request_builder.query(&[("parent",
namespace.to_url_string())]);
+ }
+ let request = request_builder.build()?;
+
+ let handler = |response: Response| async move {
+ match response.status() {
+ StatusCode::OK => {
+
Ok(deserialize_catalog_response::<ListNamespaceResponse>(response).await?)
+ }
+ StatusCode::NOT_FOUND => Err(Error::new(
+ ErrorKind::Unexpected,
Review Comment:
This change made me think about how users can handle cases where the
namespace doesn't exist. Maybe we should add an `ErrorKind::NamespaceNotExists`?
Maybe this should be done in another PR.
##########
crates/catalog/rest/src/catalog.rs:
##########
@@ -507,78 +530,98 @@ impl Catalog for RestCatalog {
})
.build()?;
- let resp = self
- .context()
- .await?
- .client
- .query::<LoadTableResponse, ErrorResponse>(request)
- .await?;
+ let handler = |response: Response| async move {
+ match response.status() {
+ StatusCode::OK => {
+
Ok(deserialize_catalog_response::<LoadTableResponse>(response).await?)
+ }
+ StatusCode::NOT_FOUND => Err(Error::new(
+ ErrorKind::Unexpected,
+ "Tried to create a table under a namespace that does not
exist",
+ )),
+ StatusCode::CONFLICT => Err(Error::new(
+ ErrorKind::Unexpected,
Review Comment:
The same question, maybe we should have `TableNotFound` nad
`TableAlreadyExists`.
##########
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:
To be honest, I don't like the style of creating a handle and passing it
into another function. It usually breaks the logical flow and makes it hard to
understand what’s happening.
It's easy to understand that:
```rust
let resp = self
.context()
.await?
.client
.query::<CommitTableResponse, ErrorResponse>(request)
.await?;
```
Ok, the client will perform query and returns a response or error.
But it's hard to:
```rust
let handler = |response: Response| async move { ... };
let response = context.client.query_catalog(request, handler).await?;
```
Let's build a handler and it will do something to the response. Remember
this funcation now. The client will perform query catalog and use the previous
handler to parse the response. Emmmm, wait a second, what does this handler do?
---
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.
--
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: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
