feat: Allow servers to express supported endpoints with ConfigResponse #2848
Conversation
|
wdyt about adding integration tests against the Running the integration test infra gives me this response on |
kevinjqliu
left a comment
There was a problem hiding this comment.
LGTM! Thanks for adding this feature.
The PR looks good and is throughly tested. I just have a few nit comments.
Feel free to address here or as a follow up PR
| NotImplementedError: If the endpoint is not supported. | ||
| """ | ||
| if endpoint not in self._supported_endpoints: | ||
| raise NotImplementedError(f"Server does not support endpoint: {endpoint}") |
There was a problem hiding this comment.
nit: java throws UnsupportedOperationException here
There was a problem hiding this comment.
There really isn't an exact equivalent for java's UnsupportedOperationException exception in python. I went with NotImplementedError since it's the closest. Alternatively, we can go with RuntimeException.
| fetch_scan_tasks: str = "namespaces/{namespace}/tables/{table}/tasks" | ||
|
|
||
|
|
||
| class Capability: |
There was a problem hiding this comment.
nit: maybe we can refactor the Endpoints class and consolidate this class
There was a problem hiding this comment.
Initially I went with this approach, but it felt like it just introduced some confusion. Since the Capabilities seem more like a config to flag support, and the resource paths or Endpoints are similar but adding the prefix to the url would make the url construction complex because prefix is optional.
There was a problem hiding this comment.
nit: wdyt of adding the endpoints in the rest_mock here
that way we dont need to add it to each test.
We can modify it when testing specific cases, such as when an older server does not return the view endpoints, or when testing the endpoint response directly
closes to #2847
Rationale for this change
This PR adds the server endpoint capabilities support, aligning with the Java implementation. While working on the REST scanning support, we need to know if a server supports specific capabilities before making any calls. So this PR also adds some extra support for the current implementation of PI iceberg REST catalog.
The REST catalog will now parse the endpoints field from the config call to determine server capabilities. When a server doesn't respond, we have fallback logic that matches the behavior of Java's rest catalog. The View endpoints are conditionally added to the default with the config property as well.
Are these changes tested?
Added unit tests and tested with the iceberg rest fixture.
Are there any user-facing changes?
Yes added config and alignment with java impl.
cc: @kevinjqliu @Fokko