CTTY commented on code in PR #2794:
URL: https://github.com/apache/iceberg-rust/pull/2794#discussion_r3582936998
##########
crates/catalog/rest/src/catalog.rs:
##########
@@ -431,14 +434,51 @@ 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(crate) async fn supports_endpoint(&self, endpoint: &Endpoint) ->
Result<bool> {
+ Ok(self.context().await?.endpoints.contains(endpoint))
+ }
+
+ /// Issue a `HEAD` request to `url` and interpret it as an existence check:
+ /// `2xx` means it exists, `404` means it doesn't.
+ async fn check_exists_via_head(&self, url: String) -> Result<bool> {
Review Comment:
We can pass the context in directly. Otherwise we are fetching context
twice: first to get url, then to get client
##########
crates/catalog/rest/src/endpoint.rs:
##########
@@ -128,6 +142,53 @@ impl<'de> Deserialize<'de> for Endpoint {
}
}
+/// The standard v1 endpoints assumed to be supported when a server's
+/// `GET /v1/config` response omits the `endpoints` field or sends an empty
list
+/// (the two are treated alike). These are the minimum a server is expected to
+/// support; a server that advertises a non-empty list is taken at its word
+/// instead.
+pub(crate) static DEFAULT_ENDPOINTS: LazyLock<HashSet<Endpoint>> =
LazyLock::new(|| {
Review Comment:
Got some good suggestions from AI. this way each endpoint will be easier to
reference
```
macro_rules! endpoints {
($($name:ident => $method:ident $path:literal),+ $(,)?) => {
$(
pub(crate) static $name: LazyLock<Endpoint> =
LazyLock::new(|| Endpoint::new(Method::$method, $path));
)+
};
}
endpoints! {
V1_LIST_NAMESPACES => GET "/v1/{prefix}/namespaces",
V1_CREATE_NAMESPACE => POST "/v1/{prefix}/namespaces",
V1_LOAD_NAMESPACE => GET "/v1/{prefix}/namespaces/{namespace}",
V1_DELETE_NAMESPACE => DELETE "/v1/{prefix}/namespaces/{namespace}",
V1_UPDATE_NAMESPACE => POST
"/v1/{prefix}/namespaces/{namespace}/properties",
V1_NAMESPACE_EXISTS => HEAD "/v1/{prefix}/namespaces/{namespace}",
V1_LIST_TABLES => GET
"/v1/{prefix}/namespaces/{namespace}/tables",
V1_CREATE_TABLE => POST
"/v1/{prefix}/namespaces/{namespace}/tables",
V1_LOAD_TABLE => GET
"/v1/{prefix}/namespaces/{namespace}/tables/{table}",
V1_UPDATE_TABLE => POST
"/v1/{prefix}/namespaces/{namespace}/tables/{table}",
V1_DELETE_TABLE => DELETE
"/v1/{prefix}/namespaces/{namespace}/tables/{table}",
V1_TABLE_EXISTS => HEAD
"/v1/{prefix}/namespaces/{namespace}/tables/{table}",
V1_RENAME_TABLE => POST "/v1/{prefix}/tables/rename",
V1_REGISTER_TABLE => POST
"/v1/{prefix}/namespaces/{namespace}/register",
V1_REPORT_METRICS => POST
"/v1/{prefix}/namespaces/{namespace}/tables/{table}/metrics",
V1_COMMIT_TRANSACTION => POST "/v1/{prefix}/transactions/commit",
}
pub(crate) static DEFAULT_ENDPOINTS: LazyLock<HashSet<Endpoint>> =
LazyLock::new(|| {
[
&*V1_LIST_NAMESPACES,
&*V1_CREATE_NAMESPACE,
&*V1_LOAD_NAMESPACE,
&*V1_DELETE_NAMESPACE,
&*V1_UPDATE_NAMESPACE,
&*V1_LIST_TABLES,
&*V1_CREATE_TABLE,
&*V1_LOAD_TABLE,
&*V1_UPDATE_TABLE,
&*V1_DELETE_TABLE,
&*V1_RENAME_TABLE,
&*V1_REGISTER_TABLE,
&*V1_REPORT_METRICS,
&*V1_COMMIT_TRANSACTION,
]
.into_iter()
.cloned()
.collect()
});
```
##########
crates/catalog/rest/src/catalog.rs:
##########
@@ -909,24 +947,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:
Same here, can we make this static?
##########
crates/catalog/rest/src/catalog.rs:
##########
@@ -648,24 +688,22 @@ impl Catalog for RestCatalog {
}
async fn namespace_exists(&self, ns: &NamespaceIdent) -> Result<bool> {
- let context = self.context().await?;
-
- let request = context
- .client
- .request(Method::HEAD, context.config.namespace_endpoint(ns))
- .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}");
Review Comment:
The construction is per-call. Can we make this static?
--
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]
