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
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
| if not raw_path: | ||
| raise ValueError("Invalid path: empty") | ||
| raw_path = raw_path.strip() | ||
| if not raw_path: | ||
| raise ValueError("Invalid path: empty") |
There was a problem hiding this comment.
| if not raw_path: | |
| raise ValueError("Invalid path: empty") | |
| raw_path = raw_path.strip() | |
| if not raw_path: | |
| raise ValueError("Invalid path: empty") | |
| raw_path = raw_path.strip() | |
| if not raw_path: | |
| raise ValueError("Invalid path: empty") |
i think we can just check once here
| 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
| return f"{self.http_method.value} {self.path}" | ||
|
|
||
| @classmethod | ||
| def from_string(cls, endpoint: str | None) -> "Endpoint": |
There was a problem hiding this comment.
| def from_string(cls, endpoint: str | None) -> "Endpoint": | |
| def from_string(cls, endpoint: str) -> "Endpoint": |
can we enforce that endpoint must be str?
| def from_string(cls, endpoint: str | None) -> "Endpoint": | ||
| if endpoint is None: | ||
| raise ValueError("Invalid endpoint (must consist of 'METHOD /path'): None") | ||
| elements = endpoint.split(None, 1) |
There was a problem hiding this comment.
| elements = endpoint.split(None, 1) | |
| elements = endpoint.strip().split(None, 1) |
strip leading/trailing whitespace before split, just in case
| 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
|
|
||
| if Capability.V1_NAMESPACE_EXISTS not in self._supported_endpoints: | ||
| try: | ||
| self.load_namespace_properties(namespace_tuple) | ||
| return True | ||
| except NoSuchNamespaceError: | ||
| return False | ||
|
|
There was a problem hiding this comment.
took me a while to find this logic 😄 could we add a note similar here similar to java's
// fallback in order to work with 1.7.x and older servers
| Returns: | ||
| bool: True if the table exists, False otherwise. | ||
| """ | ||
| if Capability.V1_TABLE_EXISTS not in self._supported_endpoints: |
There was a problem hiding this comment.
same as above, add a note about this special case
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
2 participants
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