-
Notifications
You must be signed in to change notification settings - Fork 176
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
Separate Polaris-native API from Iceberg Rest Catalog API spec #906
Conversation
# Conflicts: # service/common/src/main/java/org/apache/polaris/service/catalog/IcebergCatalogAdapter.java
I thought we do not want to have two PRs #906 and #856 for the same thing (see #884 and https://lists.apache.org/thread/bcnh1dwgoxd2dvtqk6z935gfzmh4q0jq) |
@snazy Thanks for bringing this up. Sorry for the confusion, the 2 PR should for the different changes but I forgot to update the title. I will pay attention to this in the future to better follow the guideline |
|
||
Whenever the source specification files are updated, the generated files must be re-generated to reflect those changes. | ||
|
||
Below are steps to generate `bundled-polaris-catalog-service.yaml` |
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.
do we need to also commit the generated file? If possible I think we should have that in .gitignore
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.
Yeah, it is a weird but it is necessary for Polaris's hugo site to render preview.
https://github.com/apache/polaris/blob/main/site/content/in-dev/unreleased/rest-catalog-open-api.md?plain=1#L27
{{- $url = printf "https://raw.githubusercontent.com/apache/polaris/refs/heads/main/spec/%s" $spec }} |
The site need a url of the yaml to render the preview: https://polaris.apache.org/in-dev/unreleased/rest-catalog-open-api/, so we have to push it to the github
We can remove the generated one from github if our site can render a local yaml, but that to my current understanding will require non-trivial change to our site so we may explore later.
- `polaris-catalog-service.yaml` - Defines the specification for the Polaris Catalog API, which encompasses both the Iceberg REST Catalog API | ||
and Polaris-native API. | ||
- `polaris-apis` - Contains the specifications of Polaris-native API | ||
- `rest-catalog-open-api.yaml` - Contains the specification for Iceberg Rest Catalog API |
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.
we might want to rename this as iceberg-rest-catalog-open-api.yaml
just to be clear.
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.
Good point! I plan to do the rename in a follow-up PR because
- The current
rest-catalog-open-api.yaml
is referenced in our hugo site setting - We need to extract additional stuff from
rest-catalog-open-api.yaml
to make it match a released version of API.
I think we can do the above 2 together in one separate PR so the current one won't contain too many changes
# Polaris-native API # | ||
###################### | ||
|
||
/v1/{prefix}/namespaces/{namespace}/tables/{table}/notifications: |
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.
this does not include the management APIs?
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.
The Management APIs and Catalog APIs have different scopes and are typically referenced separately for service/client generation, as seen here:
https://github.com/HonahX/polaris/blob/4c7ae00b542806928dc05d1e4f56d51014dedac5/api/iceberg-service/build.gradle.kts#L51
https://github.com/HonahX/polaris/blob/4c7ae00b542806928dc05d1e4f56d51014dedac5/api/management-service/build.gradle.kts#L48
They also have different URL prefixes, security configurations, etc. Given these differences, I think it makes sense to keep them in separate YAML files for better flexibility.
The goal of this PR is to clearly separate Polaris-specific catalog APIs from Iceberg catalog APIs.
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.
looks good to me!
These files should not be manually edited (except adding license header). They are intended for preview purposes only, | ||
such as rendering a preview on a website. | ||
|
||
Whenever the source specification files are updated, the generated files must be re-generated to reflect those changes. |
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.
Is re-generation manual or part of build process? I think we could make it part of build process if it's manual. Not a blocker for this PR though.
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.
Currently it's manual. I think once we find a way to render local rest spec YAML instead of referring to GitHub main branch one, we can make this automatic.
Thanks @HonahX for working on it. Thanks @snazy @jackye1995 for the review. |
Not ready for review
This PR fixes: #553 by separating Polaris-native catalog APIs from Iceberg REST Catalog spec. Dev list discussion: https://lists.apache.org/thread/1fqocs00pno0xfr4ss2p69d6dv5h8qzf
polaris-apis/notifications-api.yaml
. The new yaml contains necessary paths and components def for the notifications APIpolaris-catalog-service.yaml
which will be the root spec file that groups IRC Apis fromrest-catalog-open-api.yaml
andpolaris-apis/notifications-api.yaml
. This one will be used in the builder to generate open api service.generated/bundled-polaris-catalog-service.yaml
for website to render the preview of APIsThere will be 2 follow-ups of this PR
generated/bundled-polaris-catalog-service.yaml
rest-catalog-open-api.yaml
match the released version (currently 1.7.1)