Skip to content

Polaris Catalog Spec: Policy Management Apis#808

Merged
flyrain merged 5 commits intoapache:mainfrom
HonahX:honahx_policy_management_apis
Mar 14, 2025
Merged

Polaris Catalog Spec: Policy Management Apis#808
flyrain merged 5 commits intoapache:mainfrom
HonahX:honahx_policy_management_apis

Conversation

@HonahX
Copy link
Copy Markdown
Contributor

@HonahX HonahX commented Jan 16, 2025

The PR adds definition of policy management APIs designed in https://docs.google.com/document/d/1kIiVkFFg9tPa5SH70b9WwzbmclrzH3qWHKfCKXw5lbs/edit?tab=t.0#heading=h.nly223xz13km

The first set of APIs includes:

  • createPolicy
  • getPolicy
  • updatePolicy
  • dropPolicy
  • listPolicies
  • attachPolicy
  • detachPolicy

This PR also provides a models and example result to serve as a start point of discussion. All implementations and definition are subject to be changed upon further discussion.

Convenient viewer: https://editor-next.swagger.io/?url=https://raw.githubusercontent.com/HonahX/polaris/refs/heads/honahx_policy_management_apis/spec/generated/bundled-polaris-catalog-service.yaml

cc: @flyrain

@HonahX HonahX changed the title [WIP] Spec: Policy Management Apis [WIP] Rest Spec: Policy Management Apis Jan 16, 2025
@HonahX HonahX changed the title [WIP] Rest Spec: Policy Management Apis [WIP] Catalog Spec: Policy Management Apis Jan 16, 2025
@HonahX HonahX changed the title [WIP] Catalog Spec: Policy Management Apis Catalog Spec: Policy Management Apis Jan 22, 2025
@HonahX HonahX marked this pull request as ready for review January 23, 2025 03:59
@HonahX
Copy link
Copy Markdown
Contributor Author

HonahX commented Jan 23, 2025

I also created a draft PR that use a separate yaml to hold policy management APIs: #856

Comment thread api/iceberg-service/build.gradle.kts Outdated
Comment thread spec/rest-catalog-open-api.yaml Outdated
5XX:
$ref: '#/components/responses/ServerErrorResponse'

/v1/{prefix}/namespaces/{namespace}/policies:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why policy need to be under a namespace? why not directly /v1/{prefix}/policies? Curious if there are other similar systems and examples.

Copy link
Copy Markdown
Contributor

@jackye1995 jackye1995 Jan 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like from this comment we actually prefer to have policy directly under catalog, at least for now

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here are reasons why policies should not be top-level entities in a Polaris instance:

  1. Policies need a clear ownership structure, either under a catalog or a namespace, to simplify lifecycle management. For example, you can delete all policies associated with a catalog or namespace when they are no longer attached to any entities outside their owner. Typically, there’s a dedicated team responsible for creating and attaching policies to resource entities, like tables. This team usually doesn’t own any tables. This design also aligns with other systems like Snowflake and Dremio, which also put policies under a catalog or namespace.
  2. In Polaris, each catalog maintains its own catalog roles with assigned privileges. Privileges are not granted directly to principal roles; instead, they are granted to catalog roles, which are then assigned to principal roles. With this privilege structure, it is more flexible to put policies under a catalog/namespace.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When it comes to deciding whether policies should live under catalogs or namespaces, there’s no definitive answer—it’s more about implementation preferences and what fits best for the system. That said, looking at examples from systems like Snowflake and Dremio, they both place policies under namespaces, and there are some clear benefits to doing so. It provides consistency in entity organization. Tables and views are "real" objects, the actual data entities, while catalogs and namespaces serve as layers to organize them. By placing policies under namespaces, you maintain this hierarchy, keeping things clean and logical.

We organically want polices owned by a catalog as you can see from the design doc, due to the reason that we don’t allow applying policies across-catalogs. But we removed that limit later, so that putting them under a catalog isn’t compelling any more, as we still have to check if all policies are used when we delete them as a batch. We can still put them under a catalog, but it means there are two sets of endpoints for policy CRUD. With that, I think it is fine to put polices under a namespace as the initial implementation. We could add new endpoints if needed in the future.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally, I would start with policies at the Catalog level, with the ability to add policy overrides at the namespace level later on. It is more consistent with how we manage privileges and storage configuration. Both start at the catalog level and overrides are attached to the child entities. Given that the mapping still happens at the namespace, I think CRUD at the Catalog level makes sense.

Copy link
Copy Markdown
Contributor

@flyrain flyrain Feb 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me break down the difference between policy ownership and policy assignment a bit.

Policy Ownership:
Think of this as where the policy "lives" and how it's managed. For example, a policy might be identified as c1.ns1.policy1, meaning it's part of a specific namespace. When you delete a namespace, you need to clean up everything under it—including the policies. This helps keep things organized. Also, in many companies, there's a dedicated team that's responsible for defining policies. They don't own any tables themselves, but they do manage the policies under a namespace (or something similar like a schema). Grouping policies under a namespace works well in this setup.

Policy Assignment:
Just because a policy exists in a namespace doesn't mean it automatically applies there. You have to actively assign the policy to a specific resource, like a catalog, namespace, or table. Think of it like having a rule book that's stored in a specific area, but you only use a rule when you decide to apply it to a particular situation. The following diagram shows how this assignment works in practice.

Another key reason for placing policies under a namespace is that it reinforces the purpose of the namespace: grouping real objects. If policies were placed directly under a catalog, they would become mixed with the grouping layer, potentially increasing the complexity and maintenance burden of the system. This approach aligns with practices used by other systems such as Snowflake, Dremio and Unity Catalog.
Screenshot 2025-02-14 at 1 46 02 PM

snazy
snazy previously requested changes Jan 27, 2025
Copy link
Copy Markdown
Member

@snazy snazy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change modifies Iceberg's REST API and adds Polaris specifics to it. Nothing guarantees that this "patch" on top of Iceberg's REST API won't break with an evolution of Iceberg's REST API.

There are also Polaris specific notions like "catalog" added - that concept doesn't exist in the Iceberg REST API. Referencing one catalog from another catalog from the Iceberg REST API looks strange to me.

Values, especially those used for path-parameters, should have validation patterns and only allow unproblematic characters.

Documentation for types, operations, fields should be more verbose - examples should also be added.

The new types NamespaceIdentifier and TableLikeIdentifier already exist in the same spec. Not sure whether duplicating those types makes sense.

I'd prefer to omit using any enum - that translates to a Java enum, which will break with later added enum values very early during request processing. (Not safe for future development.)

A policy is defined as PolicyContent: {}, which means that anything can go in there. It is rather impossible for a service to properly handle "concurrent" updates to a policy - the latest one would blindly win and silently overwrite all other changes. Why not use "patch" here?

I also recommend to not rely on a "version" attribute. The intent of it is undefined. Similar for created/updated-at-ms.

A policy has an ID, a name and description. It seems like ID is a unique, technical ID and name a human friendly name. However, only the description can be updated.

Overall, I think this PR needs some more changes, particularly documentation and examples and clear and types that can be validated. This change should IMHO also not update the Iceberg REST API.

Copy link
Copy Markdown
Contributor Author

@HonahX HonahX left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@snazy Thanks a lot for the detailed review and all the suggestions! If you don’t mind, I’d like to separate my reply into different threads.

This change modifies Iceberg's REST API and adds Polaris specifics to it. Nothing guarantees that this "patch" on top of Iceberg's REST API won't break with an evolution of Iceberg's REST API.

Thanks for bringing this! I totally agree we should start to separate out Polaris only APIs from IRC YAML, keeping the yaml consistent with the upstream released version. I think this also related to the recent dev list discussion: https://lists.apache.org/thread/1fqocs00pno0xfr4ss2p69d6dv5h8qzf. I just sent a reply there proposing a possible solution to separate apis into different YAMLs. The PoC PR is there: #856.

Given that big changes are likely required in separating APIs and we might discuss and explore more solutions, my initial plan was to do that in a follow-up PR so that we can unblock the addition of policy management APIs. Do you think it is ok to proceed to add these APIs to the yaml as a temporary solution and update later? It will not break things as we do not include implementation for these APIs yet.

catalog: '#/components/schemas/CatalogIdentifier'
namespace: '#/components/schemas/NamespaceIdentifier'
table-like: '#/components/schemas/TableLikeIdentifier'
properties:
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are also Polaris specific notions like "catalog" added - that concept doesn't exist in the Iceberg REST API. Referencing one catalog from another catalog from the Iceberg REST API looks strange to me.

I agree. I think this also aligns with the need for YAML separation. Separating Polaris APIs from the IRC spec file will also help extend the concept of 'catalog' in Polaris, that it is not just the Iceberg Rest Catalog, but Iceberg REST Catalog + Polaris-powered capabilities like policy management and volumes/directory tables. This approach will make more sense once it's moved out of the IRC YAML.

Comment thread spec/rest-catalog-open-api.yaml Outdated
policy:
name: policy
in: path
description: a policy name
Copy link
Copy Markdown
Contributor Author

@HonahX HonahX Jan 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Values, especially those used for path-parameters, should have validation patterns and only allow unproblematic characters.

Thanks for pointing out! I will add relevant restrictions to these values: https://datatracker.ietf.org/doc/html/rfc3986#section-2.3

Comment thread spec/rest-catalog-open-api.yaml Outdated
summary: 'Create a policy in the given namespace'
operationId: createPolicy
description:
Create a policy in the given namespace
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Documentation for types, operations, fields should be more verbose - examples should also be added.

Good point! Will add those

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added docs for new APIs

Comment thread spec/rest-catalog-open-api.yaml Outdated
type: string

NamespaceIdentifier:
allOf:
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new types NamespaceIdentifier and TableLikeIdentifier already exist in the same spec. Not sure whether duplicating those types makes sense.

These are added to support attaching policies to an entity in a different catalog. I think @flyrain can provide more context on this use case. So the additional catalog field here is what it differentiate from the existing TableIdentifier and Namespace.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We will always need the full path to support cross-catalog policy attachment. For example, we will need an identifier like c1.ns1 to make sure a policy can be attached to the namespace ns1 under catalog c1. The exiting schema namespace doesn't contain the full path. We will need NamespaceIdentifier.

TableLikeIdentifier is needed as policy could be applied to any table-like entities, like views, volumes(WIP)

Comment thread spec/rest-catalog-open-api.yaml Outdated
properties:
type:
type: string
enum:
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer to omit using any enum - that translates to a Java enum, which will break with later added enum values very early during request processing. (Not safe for future development.)

I got the idea from

GrantResource:
type: object
discriminator:
propertyName: type
mapping:
catalog: '#/components/schemas/CatalogGrant'
namespace: '#/components/schemas/NamespaceGrant'
table: '#/components/schemas/TableGrant'
view: '#/components/schemas/ViewGrant'
properties:
type:
type: string
enum:
- catalog
- namespace
- table
- view
required:
- type

Do we also want to update that one?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed the enum for EntityIdentifier

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @snazy , I gave this a second thought, and I think keeping the enum could actually be helpful. The polymorphism it provides helps us validate Identifier types and ensures we only allow these valid field combinations:

  • CatalogIdentifier → catalog
  • NamespaceIdentifier → catalog, namespace
  • TableLikeIdentifier → catalog, namespace, name

That said, I’d like to understand more about the risks you mentioned regarding enums being "not safe for future development." My current understanding is that if we add new enum values later, older servers would simply reject them as unsupported—so I’m curious if there’s something else I’m missing. Would appreciate any insights!

Comment thread spec/rest-catalog-open-api.yaml Outdated
type: integer
format: int64

PolicyContent: {}
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A policy is defined as PolicyContent: {}, which means that anything can go in there. It is rather impossible for a service to properly handle "concurrent" updates to a policy - the latest one would blindly win and silently overwrite all other changes. Why not use "patch" here?

The design is that PolicyContent can be any format, as long as it can be validated by the validator of the corresponding policy type. (doc). For example, a string of some SQL string can be the whole "PolicyContent" so we will need to replace the whole thing in every update. For user-defined custom policy type, we will not know what's in the policy content, so we will rely on the provided validator to ensure the policy is in a good shape.

I think I should add some basic introduction of Policy Validation in the spec to avoid confusion.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated the PolicyContent to be a string which can hold JSON string, SQL, or any other format.

Comment thread spec/rest-catalog-open-api.yaml Outdated
type: string
content:
$ref: '#/components/schemas/PolicyContent'
version:
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also recommend to not rely on a "version" attribute. The intent of it is undefined. Similar for created/updated-at-ms.

For version do you think a more meaningful name, such as policy-version and some additional description of what policy-version looks like and its usage make it better here? In the future, we will also introduce APIRestorePolicyVersion to help rollback.

For created/updated-at-ms, I thought they were for informative purpose only. Do you recommend to switch to some other format to represent this information or to not include these information in the object?

Thanks in advance!

Comment thread spec/rest-catalog-open-api.yaml Outdated
UpdatePolicyRequest:
type: object
properties:
description:
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A policy has an ID, a name and description. It seems like ID is a unique, technical ID and name a human friendly name. However, only the description can be updated.

The ID is unique. The current design is that policy name is also required to be unique within a namespace so that we can let user provide policy identifier (namespace + name) when loading. I am thinking of adding another endpoint to rename policy, similar as /v1/{prefix}/tables/rename. WDYT?

Copy link
Copy Markdown
Contributor

@flyrain flyrain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @HonahX for the PR, left some comments.

Comment thread spec/rest-catalog-open-api.yaml Outdated
Comment thread spec/rest-catalog-open-api.yaml Outdated
Comment thread spec/rest-catalog-open-api.yaml Outdated
Comment thread spec/rest-catalog-open-api.yaml Outdated
Comment thread spec/rest-catalog-open-api.yaml Outdated
@HonahX HonahX requested a review from MonkeyCanCode as a code owner February 3, 2025 18:47
@HonahX
Copy link
Copy Markdown
Contributor Author

HonahX commented Feb 5, 2025

Thanks everyone for reviewing it. Now that #906 is merged, we can separate the policy management APIs into a dedicated file: spec/polaris-catalog-apis/policy-management-apis.yaml. I've updated the PR accordingly and addressed the review comments.
@flyrain @collado-mike @snazy @jackye1995 Would you mind taking another look when you have a chance? Appreciate your help!

Comment on lines +1378 to +1381
- `name`(REQUIRED): The name of the policy. It Must include only unreserved URL characters: letters(A-Z, a-z), digits (0-9), hyphens (-), periods (.), underscores (_) and tildes (~)
- `type` (REQUIRED): The type of the policy. It can be either predefined type or custom type.
- **Predefined Policies:** system.compaction, system.snapshot_retention
- **Custom Policies:** custom.<org_name>.data_masking, custom.<user_id>.audit_policy
Copy link
Copy Markdown
Contributor

@flyrain flyrain Feb 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm now questioning myself if it's a good idea to allow . in policy name. The concern is that how do we address a policy. Ideally it should be addressed similar to a table identifier(c1.ns1.t1). With a dot in the name, a name like c1.ns1.a.b is ambiguous, esp. that Polaris supports multiple level of namespace. We don't know if a is a namespace or part of policy name.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the confusion, I think we should just make sure policy name doesn't have dot. policy type is fine.

Copy link
Copy Markdown
Contributor Author

@HonahX HonahX Feb 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! I removed the dot here, and also ~, so we only allow commonly used characters.

Comment thread spec/polaris-catalog-apis/policy-management-apis.yaml Outdated
Comment thread spec/polaris-catalog-apis/policy-management-apis.yaml Outdated
@HonahX HonahX requested a review from collado-mike March 12, 2025 21:02
Copy link
Copy Markdown
Contributor

@flyrain flyrain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @HonahX for working on it. Looks pretty good overall. Left minor comments.

Comment thread spec/polaris-catalog-service.yaml Outdated
Comment thread spec/polaris-catalog-apis/policy-apis.yaml Outdated
Comment thread spec/polaris-catalog-apis/policy-apis.yaml Outdated
Comment on lines 354 to 361
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it's fine to infer the type this way. So that we save one parameter.

Comment thread spec/polaris-catalog-apis/policy-apis.yaml Outdated
Comment thread spec/polaris-catalog-apis/policy-apis.yaml Outdated
Comment thread spec/polaris-catalog-apis/policy-apis.yaml Outdated
Comment thread spec/polaris-catalog-service.yaml Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's just call this models, then later we can merge the ones for generic table and yours in one

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about we define policyManagementModels and genericTableModels and then then merge them into the generator configuration? This approach clearly shows where each model originates.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, that sounds good to me.

Comment thread spec/polaris-catalog-apis/policy-apis.yaml Outdated
Comment thread spec/polaris-catalog-apis/policy-apis.yaml Outdated
Comment thread spec/polaris-catalog-apis/policy-apis.yaml Outdated
Comment thread spec/polaris-catalog-apis/policy-apis.yaml Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that name is quite long, would detach-all work?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe even just detach?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great suggestion! Let's use the short one

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After some thoughts, I prefer to use detach-all because it clearly indicates that the policy will be detached from all targets when this parameter is set to true.

Comment thread spec/polaris-catalog-apis/policy-apis.yaml Outdated
Comment thread spec/polaris-catalog-apis/policy-apis.yaml Outdated
Comment thread spec/polaris-catalog-apis/policy-apis.yaml Outdated
Comment thread spec/polaris-catalog-service.yaml Outdated
Comment on lines 141 to 151
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't we keep all of these defined in the policy-apis.yaml file?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @collado-mike, similar to #1150 (comment), I currently put here to make sure we do not duplicate servers and security schemes definition. And this also means we can easily render a preview of all supported API like https://editor-next.swagger.io/?url=https://raw.githubusercontent.com/HonahX/polaris/refs/heads/honahx_policy_management_apis/spec/generated/bundled-polaris-catalog-service.yaml

But I agree there are also advantages of the alternative approach. How about we discuss that in a follow-up and do a refactor as a whole once generic table api and policy api are all in? We can re-organize these files any time if needed.

Comment thread spec/polaris-catalog-apis/policy-apis.yaml Outdated
Comment thread spec/polaris-catalog-apis/policy-apis.yaml Outdated
Comment on lines 207 to 208
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this param is set to false, what error code is returned on failure?

Copy link
Copy Markdown
Contributor Author

@HonahX HonahX Mar 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question! I think we should return 400 - BadRequest, similar to how we currently handle NamespaceNotEmptyException

case NamespaceNotEmptyException e -> Status.BAD_REQUEST.getStatusCode();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please include this in the spec itself

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe even just detach?

Comment thread spec/polaris-catalog-apis/policy-apis.yaml Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any restrictions around starting character? E.g., is it ok to start with something like ___mypolicy__?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’ve gone back and forth on this. Initially, I didn’t set restrictions—like with existing tables/views—but based on @snazy's suggestion, filtering out invalid characters makes sense. That said, I prefer minimal restrictions unless there's a reason. Would something like ___mypolicy__ be problematic in some future use case? Or is it a general good practice to limit the starting and trailing character to be letters or digits only?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IDK - we have no such restrictions on other entities in Polaris, so... it's whatever we want. It's just impossible to make it more restrictive later

Comment thread spec/polaris-catalog-apis/policy-apis.yaml Outdated
Comment thread spec/polaris-catalog-apis/policy-apis.yaml Outdated
Comment thread spec/polaris-catalog-apis/policy-apis.yaml Outdated
@HonahX HonahX force-pushed the honahx_policy_management_apis branch from 474c063 to 51de960 Compare March 13, 2025 19:09
@HonahX HonahX requested a review from collado-mike March 13, 2025 19:10
Copy link
Copy Markdown
Contributor

@collado-mike collado-mike left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving to unblock - please add the answers to the questions I asked into the spec itself so that readers can understand the expected behavior. People aren't going to go back and read the original design doc, nor is the design doc going to be updated with behavior changes going forward. Ambiguities like these should be addressed by the spec

@github-project-automation github-project-automation Bot moved this from PRs In Progress to Ready to merge in Basic Kanban Board Mar 14, 2025
@flyrain flyrain merged commit 495aea0 into apache:main Mar 14, 2025
5 checks passed
@github-project-automation github-project-automation Bot moved this from In Progress to Done in Policy management Mar 14, 2025
@github-project-automation github-project-automation Bot moved this from In Progress to Done in Table Maintenance Mar 14, 2025
@github-project-automation github-project-automation Bot moved this from Ready to merge to Done in Basic Kanban Board Mar 14, 2025
@snazy
Copy link
Copy Markdown
Member

snazy commented Mar 14, 2025

@flyrain looks like you oversaw my strong concern This change modifies Iceberg's REST API and adds Polaris specifics to it. Nothing guarantees that this "patch" on top of Iceberg's REST API won't break with an evolution of Iceberg's REST API. - this has not been addressed - neither in this PR nor in #1150.

To be clear: my request for changes still stands! So I ask you to revert this change!

Related to the discussion on the dev-ML: Concerns about changing and/or depending on the Iceberg REST spec have been raised multiple times on PRs and on the dev-mailing-list. This PR introduces a hard dependency on the Iceberg REST spec.

The approach take in this PR and #1150 should have discussed explicitly on the dev mailing list, as stated in the project's contributing guidelines, which explicitly say: "Change of public interface (or more generally speaking Polaris extension point) should be discussed and approved on the dev mailing list. The discussion on the dev mailing list should happen before having a “ready-for-review” Pull Request."

@flyrain
Copy link
Copy Markdown
Contributor

flyrain commented Mar 14, 2025

@snazy, thanks for raising your concerns!

@HonahX has replied and addressed your concern more than one month ago per the timestamp in these comments:
#808 (review)
#808 (comment)
#808 (comment)
#808 (comment)
#808 (comment)
#808 (comment)
#808 (comment)
#808 (comment)
#808 (comment)
#808 (comment)

With a long waiting for responses from you, we can only consider you are OK with it. The ASF lazy consensus rule( https://community.apache.org/committers/decisionMaking.html#lazy-consensus) also explicitly mentioned the 3 days decision-making process. With that, I don't think we need to revert this PR. Sorry to hear that you still think it's not resolved.
But feel free to bring up the discussion on the dev mailing list to clarify the dependencies and concerns raised.

I appreciate your vigilance and look forward to resolving concerns collaboratively.

rakesh-das08 pushed a commit to rakesh-das08/polaris that referenced this pull request Mar 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done
Status: Done

Development

Successfully merging this pull request may close these issues.

8 participants