CTTY commented on code in PR #2692:
URL: https://github.com/apache/iceberg-rust/pull/2692#discussion_r3540322250
##########
crates/catalog/rest/src/catalog.rs:
##########
@@ -884,24 +922,24 @@ impl Catalog for RestCatalog {
/// Check if a table exists in the catalog.
async fn table_exists(&self, table: &TableIdent) -> Result<bool> {
- let context = self.context().await?;
-
- let request = context
- .client
- .request(Method::HEAD, context.config.table_endpoint(table))
- .build()?;
-
- let http_response = context.client.query_catalog(request).await?;
-
- match http_response.status() {
- StatusCode::NO_CONTENT | StatusCode::OK => Ok(true),
- StatusCode::NOT_FOUND => Ok(false),
- _ => Err(deserialize_unexpected_catalog_error(
- http_response,
- context.client.disable_header_redaction(),
- )
- .await),
+ let head_endpoint = Endpoint::new(
+ Method::HEAD,
+ "/v1/{prefix}/namespaces/{namespace}/tables/{table}",
+ );
Review Comment:
Should we add static endpoint for each of them, similar to Java?
```rust
pub(crate) static V1_LIST_NAMESPACES: LazyLock<Endpoint> =
LazyLock::new(|| Endpoint::new(Method::GET, "/v1/{prefix}/namespaces"));
pub(crate) static V1_LOAD_TABLE: LazyLock<Endpoint> =
LazyLock::new(|| Endpoint::new(Method::GET,
"/v1/{prefix}/namespaces/{namespace}/tables/{table}"));
// ... one per route ...
pub(crate) static DEFAULT_ENDPOINTS: LazyLock<HashSet<Endpoint>> =
LazyLock::new(|| {
[
&*V1_LIST_NAMESPACES,
&*V1_LOAD_TABLE,
// ... the rest of the base-set routes ...
]
.into_iter()
.cloned()
.collect()
});
```
This would make each endpoint referable.
##########
crates/catalog/rest/src/catalog.rs:
##########
@@ -412,14 +415,33 @@ impl RestCatalog {
.get_or_try_init(|| async {
let client = HttpClient::new(&self.user_config)?;
let catalog_config = RestCatalog::load_config(&client,
&self.user_config).await?;
+ // Use the advertised endpoints as-is, falling back to
+ // `DEFAULT_ENDPOINTS` when absent or empty.
+ let endpoints = match &catalog_config.endpoints {
+ Some(advertised) if !advertised.is_empty() => {
+ advertised.iter().cloned().collect()
+ }
+ _ => crate::endpoint::DEFAULT_ENDPOINTS.clone(),
+ };
let config =
self.user_config.clone().merge_with_config(catalog_config);
let client = client.update_with(&config)?;
- Ok(RestContext { config, client })
+ Ok(RestContext {
+ config,
+ client,
+ endpoints,
+ })
})
.await
}
+ /// Returns whether the server supports `endpoint`, per the `endpoints` it
+ /// advertised in `GET /v1/config` (or a default base set when it
advertised
+ /// none).
+ pub async fn supports_endpoint(&self, endpoint: &Endpoint) -> Result<bool>
{
Review Comment:
1. I saw we only gated `table_exists` and `namespace_exists`, but why not
all other operations like `list_namespaces`, `list_tables`, etc.?
2. I'm still not sure why this has to be public. exists check doesn't
require the api to be public. Maybe we can set this to `pub` once the scan
planning feature is in place and absolutely need this. wdyt?
--
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]
