feat(catalog-rest): parse server-advertised endpoints from GET /v1/config#2692
feat(catalog-rest): parse server-advertised endpoints from GET /v1/config#2692huan233usc wants to merge 1 commit into
Conversation
9f2517e to
a0837b8
Compare
8d72566 to
c769500
Compare
amogh-jahagirdar
left a comment
There was a problem hiding this comment.
Overall looks great @huan233usc just had some questions on some comments and I think we probably want to handle the explicit empty case a bit differently by just falling back to the default.
c769500 to
b470b94
Compare
|
Thank you @amogh-jahagirdar and @kevinjqliu for the review! Tried addressing the comments from both! PTAL, thanks |
|
is there already an integration test setup using the iceberg-rest-fixture docker image? i think theres is, but not 100% sure. Would be great to add some integration tests agains that IRC endpoint |
fbfb4fb to
e770ac5
Compare
Yeah — crates/integration_tests runs against the iceberg-rest-fixture image. Added endpoint_negotiation_test.rs exercising supports_endpoint against the fixture's real GET /v1/config. Thanks! |
|
Hi @amogh-jahagirdar and @kevinjqliu, I tried addressing the comments, could you help taking a look when you have a chance? Thanks |
amogh-jahagirdar
left a comment
There was a problem hiding this comment.
Thanks for your patience @huan233usc , the change looks fundamentally right to me I'd just tighten up the comments in the integration tests so they're a bit less verbose. Let's give it another day or so for @kevinjqliu @CTTY or any others to review before merging
| @@ -347,6 +348,10 @@ struct RestContext { | |||
| /// | |||
| /// It's could be different from the user config. | |||
| config: RestCatalogConfig, | |||
| /// The negotiated set of endpoints used for capability negotiation: the | |||
| /// server's advertised list, or the default base set when its | |||
| /// `GET /v1/config` response omits `endpoints` or sends an empty list. | |||
| endpoints: HashSet<Endpoint>, | |||
There was a problem hiding this comment.
is it better to store endpoints inside of RestCatalogConfig? The comment leads me to believe it might be a good place
/// Runtime config is fetched from rest server and stored here.
///
/// It's could be different from the user config.
There was a problem hiding this comment.
and it seems like a lot of endpoint related functions are in there
iceberg-rust/crates/catalog/rest/src/catalog.rs
Lines 168 to 341 in 22e80a6
There was a problem hiding this comment.
Good catch — that comment was misleading. config here is actually the user config merged with the server's config overrides (not raw server state), so it's not quite the right home for the negotiated endpoints. I clarified both field comments to make the distinction clear. Happy to move it if you'd still prefer, though!
3a5f4f4 to
341030a
Compare
CTTY
left a comment
There was a problem hiding this comment.
Generally looks good! Left some questions
| "/v1/{prefix}/namespaces/{namespace}/tables/{table}/metrics", | ||
| ), | ||
| (Method::POST, "/v1/{prefix}/transactions/commit"), | ||
| ] |
There was a problem hiding this comment.
Some comments would make this more readable like
(Method::GET, "/v1/{prefix}/namespaces"), // v1 list namespaces
We can refer to java's naming
There was a problem hiding this comment.
Done — annotated each entry with its operation name. Thanks!
| /// 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> { |
There was a problem hiding this comment.
Why do we need this api public?
There was a problem hiding this comment.
Also not sure if there is any blockers that are preventing us from enforcing the supported endpoint? (Checking supported endpoints for every catalog operations) Just storing the endpoints while not using them seems weird.
If we do plan to enforce them, when should we do that?
There was a problem hiding this comment.
Kept it pub since it's now used by the exists checks (below) and is what scan planning will build on.
There was a problem hiding this comment.
Great point. table_exists and namespace_exists now use it — they issue the HEAD only when the server advertises it, and fall back to a GET load otherwise (mirrors Java's RESTSessionCatalog). Added tests for both paths. Thanks!
There was a problem hiding this comment.
-
I saw we only gated
table_existsandnamespace_exists, but why not all other operations likelist_namespaces,list_tables, etc.? -
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
pubonce the scan planning feature is in place and absolutely need this. wdyt?
There was a problem hiding this comment.
maybe we can keep this PR lean and just introduce changes related to the Endpoint, we can follow up with the gates. might make reviews easier. WDYT?
39ce037 to
9171745
Compare
9171745 to
fb2f5b2
Compare
| /// 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> { |
There was a problem hiding this comment.
-
I saw we only gated
table_existsandnamespace_exists, but why not all other operations likelist_namespaces,list_tables, etc.? -
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
pubonce the scan planning feature is in place and absolutely need this. wdyt?
| let head_endpoint = Endpoint::new( | ||
| Method::HEAD, | ||
| "/v1/{prefix}/namespaces/{namespace}/tables/{table}", | ||
| ); |
There was a problem hiding this comment.
Should we add static endpoint for each of them, similar to Java?
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.
…nfig The REST spec lets a server advertise the routes it supports in the `endpoints` field of its `GET /v1/config` response, so clients can negotiate optional capabilities instead of assuming every server implements every operation. The Rust client currently ignores this field. Add an `Endpoint` type (HTTP method + path template) parsed from the `"<method> <path>"` wire form via `FromStr` — which validates the method and the single-space shape — with serde delegating to it, and parse the config response's `endpoints` into `Option<Vec<Endpoint>>`. This is the building block for endpoint-gated features; enforcing the negotiated set across catalog operations is left to follow-ups. Mirrors the capability negotiation Java gained in apache/iceberg#10929.
fb2f5b2 to
a01ee2f
Compare
Which issue does this PR close?
Related to #1690 (server-side scan planning), which relies on endpoint
negotiation. This PR lands the negotiation piece on its own so it can be
reviewed independently.
What changes are included in this PR?
The REST spec lets a server advertise the routes it supports in the
endpointsfield of its
GET /v1/configresponse, so clients can negotiate optionalcapabilities instead of assuming every server implements every operation. The
Rust client currently ignores this field.
This mirrors the capability negotiation the Java client gained in
apache/iceberg#10929 (the
endpointsfield in the config response, theEndpointtype, and the default-endpoint fallback when a server omits the list).This PR:
Endpointtype (HTTP method + path template) parsed from the"<method> <path>"wire form viaFromStr— which validates the single-spaceshape and the HTTP method — with serde delegating to it. The method is stored
as a typed
http::Method(kept out of the public API;method()returns&str).endpointsintoOption<Vec<Endpoint>>so anabsent field and an explicit empty list are modelled distinctly, and stores
the negotiated set on the catalog's runtime context.
field → a standard base set of namespace/table operations is assumed (so an
older server still resolves its core operations as supported); a present list
— even an empty one — is used verbatim. Optional endpoints outside the base
set stay unsupported unless explicitly advertised.
RestCatalog::supports_endpoint(&Endpoint)to query the negotiatedset.
No existing behaviour changes for callers that don't consult
supports_endpoint.Are these changes tested?
Yes:
Endpoint:FromStr/serde round-trip, rejection of malformedinput (no / extra / leading / trailing space, empty), HTTP-method
normalization, and the default endpoint set.
mockitocatalog tests: a server that advertisesendpoints(assertingsupports_endpointis true/false for listed/unlisted routes), a server thatomits the field (base operations resolve as supported), and a server that
sends an explicit empty list (nothing is supported).