-
Notifications
You must be signed in to change notification settings - Fork 171
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
base: main
Are you sure you want to change the base?
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.
@@ -0,0 +1,29 @@ | |||
# Polaris API Specifications | |||
Polaris provides two sets of OpenAPI specifications: | |||
- `polaris-management-service.yml` - Defines the management APIs for using Polaris to create and manage Iceberg catalogs and their principals |
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.
should this also be moved to polaris-apis/polaris-management-api.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.
I’m thinking of renaming the folder to polaris-catalog-apis
to better reflect its purpose. I'd like to keep polaris-management-api.yaml
in the root of spec
, ensuring that every YAML file at the root is a complete OpenAPI spec, while those in subfolders serve as components or parts of specific endpoint groups. WDYT?
- `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.
|
||
|
||
components: | ||
securitySchemes: |
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.
I think we should directly write this out instead of pointing to the IRC spec, since from what was previously discussed, the IRC spec token endpoint is being deprecated.
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.
That’s a great point! Just to confirm my understanding: The OAuth2 scheme needs to be updated because it references the removed token endpoint.
I’ll make this explicit and add a note to ensure that if we remove the token endpoint from Polaris or change it to a test-only endpoint: #12, we remember to update the scheme as well.
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)