Skip to content
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

Merged
merged 27 commits into from
Feb 4, 2025

Conversation

HonahX
Copy link
Contributor

@HonahX HonahX commented Jan 29, 2025

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

  • Move notifications API to polaris-apis/notifications-api.yaml. The new yaml contains necessary paths and components def for the notifications API
  • Create polaris-catalog-service.yaml which will be the root spec file that groups IRC Apis from rest-catalog-open-api.yaml and polaris-apis/notifications-api.yaml. This one will be used in the builder to generate open api service.
  • Add generated bundle generated/bundled-polaris-catalog-service.yaml for website to render the preview of APIs

There will be 2 follow-ups of this PR

  1. Update hugo site's rest-catalog-open-api.md to point to the generated bundle generated/bundled-polaris-catalog-service.yaml
  2. Make rest-catalog-open-api.yaml match the released version (currently 1.7.1)

@snazy
Copy link
Member

snazy commented Jan 29, 2025

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)

@HonahX HonahX changed the title Separate Polaris-native API from Iceberg Rest Catalog API spec [WIP] Separate Polaris-native API from Iceberg Rest Catalog API spec Jan 29, 2025
@HonahX
Copy link
Contributor Author

HonahX commented Jan 30, 2025

@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`

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

Copy link
Contributor Author

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

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.

Copy link
Contributor Author

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

  1. The current rest-catalog-open-api.yaml is referenced in our hugo site setting
  2. 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:

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?

Copy link
Contributor Author

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.

Copy link

@jackye1995 jackye1995 left a 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.
Copy link
Contributor

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.

Copy link
Contributor Author

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.

@flyrain flyrain merged commit 84ee2e8 into apache:main Feb 4, 2025
5 checks passed
@flyrain
Copy link
Contributor

flyrain commented Feb 4, 2025

Thanks @HonahX for working on it. Thanks @snazy @jackye1995 for the review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use "genuine" Iceberg REST spec
4 participants