-
Notifications
You must be signed in to change notification settings - Fork 414
feat: Allow servers to express supported endpoints with ConfigResponse #2848
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
kevinjqliu
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! One more comment about tests
We should probably test against the IRC server for integration test, maybe in tests/integration/test_rest_catalog.py. Can add to this PR or as a follow up
|
Thanks! |
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