Core: Allow servers to express supported endpoints via endpoint field in ConfigResponse #10929

nastra · 2024-08-13T09:11:22Z

No description provided.

core/src/main/java/org/apache/iceberg/rest/Endpoint.java

core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java

dimas-b · 2024-08-15T20:27:29Z

core/src/main/java/org/apache/iceberg/rest/Endpoint.java

+ * resource path as defined in the Iceberg OpenAPI REST specification without parameter
+ * substitution, such as <b>/v1/{prefix}/namespaces/{namespace}</b>.
+ */
+public class Endpoint {


Suggestion: this could probably be an enum + a (derived) Map<String, enum> for loasing server config responses. The spec defines a fixed representation for every value, so parsing is not really necessary, I think, the spec'd values can be keys in that map.

yeah good point and I'll probably change that once we agree on the final JSON representation for a single endpoint

@nastra : Are you still ok with the enum suggestion? I think it will match the corresponding REST API spec change (#10928) closer than attempting to parse and compare for equality by entpoint components (HTTP verb and path).

I'm good either way here, the parsing is relatively trivial and doesn't seem brittle for equality comparisons.

I think currently I'm preferring the non-enum approach (basically what's in the PR right now)

Then how about not parsing the endpoint IDs? I think the spec makes them unambiguous "in whole". Avoiding parsing and component-wise comparisons, would make the processing of those IDs more robust, IMHO.

@dimas-b we do perform equality checks to make sure that an endpoint is compared as a "whole". A valid endpoint must be of the form GET /resource/path, which is separated by a single space and the http method must be uppercase + a valid HTTP method, so I'm not sure what the concern here is. Also worth mentioning that all of this is verified in TestEndpoint

core/src/main/java/org/apache/iceberg/rest/ResourcePaths.java

core/src/main/java/org/apache/iceberg/rest/Endpoint.java

core/src/main/java/org/apache/iceberg/rest/RESTTableOperations.java

core/src/main/java/org/apache/iceberg/rest/ResourcePaths.java

core/src/main/java/org/apache/iceberg/rest/responses/ConfigResponse.java

amogh-jahagirdar · 2024-09-10T16:09:25Z

core/src/main/java/org/apache/iceberg/rest/Endpoint.java

+ * resource path as defined in the Iceberg OpenAPI REST specification without parameter
+ * substitution, such as <b>/v1/{prefix}/namespaces/{namespace}</b>.
+ */
+public class Endpoint {


I'm good either way here, the parsing is relatively trivial and doesn't seem brittle for equality comparisons.

core/src/main/java/org/apache/iceberg/rest/Endpoint.java

core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java

core/src/main/java/org/apache/iceberg/rest/RESTTableOperations.java

… in ConfigResponse

nastra · 2024-09-13T16:04:03Z

I'll go ahead and merge this, thanks everyone for the feedback/reviews

danielhumanmod · 2024-10-17T03:49:28Z

Hi, may I know which version of Iceberg will contains this commit? I’m working on a feature in Apache Polaris to support endpoints in /v1/config. Understanding the release timeline for this feature would help us better planning our Iceberg dependency upgrade :)

nastra · 2024-10-17T06:07:24Z

@danielhumanmod this will be shipped with Iceberg 1.7.0

… in ConfigResponse (apache#10929)

github-actions bot added the core label Aug 13, 2024

nastra force-pushed the endpoint-discovery branch from 18cedde to 596b7eb Compare August 13, 2024 09:14

nastra changed the title ~~Core: Add endpoints field to ConfigResponse~~ Core: Allow servers to express supported endpoints via endpoint field in ConfigResponse Aug 13, 2024

nastra marked this pull request as draft August 13, 2024 09:15

nastra force-pushed the endpoint-discovery branch from 596b7eb to d343cbf Compare August 13, 2024 09:28