jerryshao commented on code in PR #13355:
URL: https://github.com/apache/gravitino/pull/13355#discussion_r4059761865
##########
docs/manage-policies-in-gravitino.md:
##########
@@ -217,42 +217,15 @@ client.deletePolicy("retention_30d");
## Object Operations
-### Attach and Detach Policies
-
-Both happen in one request, and either list can be omitted. Catalogs, schemas,
tables, filesets,
-topics, models, views, and functions can carry a policy.
-
-<Tabs groupId='language' queryString>
-<TabItem value="shell" label="REST">
-
-```shell
-curl -X POST -H "Accept: application/vnd.gravitino.v1+json" \
- -H "Content-Type: application/json" -d '{
- "policiesToAdd": ["retention_30d"],
- "policiesToRemove": ["retention_7d"]
-}' http://localhost:8090/api/metalakes/test/objects/catalog/catalog1/policies
-```
-
-</TabItem>
-<TabItem value="java" label="Java">
-
-```java
-Catalog catalog = client.loadCatalog("catalog1");
-catalog.supportsPolicies().associatePolicies(
- new String[] {"retention_30d"},
- new String[] {"retention_7d"});
-
-Schema schema = catalog.asSchemas().loadSchema("schema1");
-schema.supportsPolicies().associatePolicies(new String[] {"retention_30d"},
null);
-```
-
-</TabItem>
-</Tabs>
+Object policies are read-only results derived from effective tags. To change
the policies that apply
+to an object, associate a policy with a tag and then assign or remove that tag
on the object or one
+of its ancestors. See [Manage tags in
Gravitino](./manage-tags-in-gravitino.md) for tag assignment
+operations.
### List Policies on an Object
-The response includes policies inherited from ancestors. With `details=true`
each policy carries an
-`inherited` field, which a plain name listing does not.
+The response includes policies derived from effective tags assigned to the
object or its ancestors.
+With `details=true`, the response returns full policy objects instead of
policy names.
Review Comment:
[Nit] The rewrite drops the note about the `inherited` field, which the
endpoint still returns.
`MetadataObjectPolicyOperations.listPoliciesForMetadataObject` maps every
entity through `PolicyOperations.toDTO(policy,
Optional.of(policy.inherited().orElse(false)))`
(`server/src/main/java/org/apache/gravitino/server/web/rest/MetadataObjectPolicyOperations.java:107-113`),
and `ObjectPolicyResolver.MatchState.policy()` sets it to `true` when the only
matching tag came from an ancestor. Suggest keeping a sentence such as: "With
`details=true` each policy carries an `inherited` field, which is `true` when
the policy is matched only through a tag assigned to an ancestor."
Verified by: read the server endpoint and
`ObjectPolicyResolver.java:145-160` on this branch.
##########
core/src/main/java/org/apache/gravitino/policy/PolicyManager.java:
##########
@@ -302,150 +252,7 @@ public PolicyEntity[] listPolicyInfosForMetadataObject(
MetadataObjectUtil.checkMetadataObject(metalake, metadataObject);
checkMetalake(NameIdentifier.of(metalake), entityStore);
- Map<Long, PolicyEntity> policiesById = new LinkedHashMap<>();
- Arrays.stream(listDirectPoliciesForMetadataObject(metalake,
metadataObject, false))
- .forEach(policy -> policiesById.put(policy.id(), policy));
-
- Arrays.stream(objectPolicyResolver.resolve(metalake, metadataObject))
- .forEach(
- policy -> {
- if (policy.inherited().orElse(false)) {
- policiesById.putIfAbsent(policy.id(), policy);
- } else {
- policiesById.put(policy.id(), policy);
- }
- });
-
- for (MetadataObject parent :
MetadataObjectUtil.getParentMetadataObjects(metadataObject)) {
- Arrays.stream(listDirectPoliciesForMetadataObject(metalake, parent,
true))
- .forEach(policy -> policiesById.putIfAbsent(policy.id(), policy));
- }
- return policiesById.values().stream()
- .filter(PolicyEntity::enabled)
- .toArray(PolicyEntity[]::new);
- }
-
- @Override
- public String[] associatePoliciesForMetadataObject(
- String metalake,
- MetadataObject metadataObject,
- String[] policiesToAdd,
- String[] policiesToRemove) {
- Preconditions.checkArgument(
-
SUPPORTED_METADATA_OBJECT_TYPES_FOR_POLICIES.contains(metadataObject.type()),
- "Cannot associate policies for unsupported metadata object type %s",
- metadataObject.type());
-
- NameIdentifier entityIdent = MetadataObjectUtil.toEntityIdent(metalake,
metadataObject);
- Entity.EntityType entityType =
MetadataObjectUtil.toEntityType(metadataObject);
-
- MetadataObjectUtil.checkMetadataObject(metalake, metadataObject);
-
- // Remove all the policies that are both set to add and remove
- Set<String> policiesToAddSet =
- policiesToAdd == null ? Sets.newHashSet() :
Sets.newHashSet(policiesToAdd);
- Set<String> policiesToRemoveSet =
- policiesToRemove == null ? Sets.newHashSet() :
Sets.newHashSet(policiesToRemove);
- Set<String> common = Sets.intersection(policiesToAddSet,
policiesToRemoveSet).immutableCopy();
- policiesToAddSet.removeAll(common);
- policiesToRemoveSet.removeAll(common);
-
- NameIdentifier[] policiesToAddIdent =
- policiesToAddSet.stream()
- .map(p -> NameIdentifierUtil.ofPolicy(metalake, p))
- .toArray(NameIdentifier[]::new);
- NameIdentifier[] policiesToRemoveIdent =
- policiesToRemoveSet.stream()
- .map(p -> NameIdentifierUtil.ofPolicy(metalake, p))
- .toArray(NameIdentifier[]::new);
-
- checkMetalake(NameIdentifier.of(metalake), entityStore);
- return TreeLockUtils.doWithTreeLock(
- entityIdent,
- LockType.READ,
- () ->
- TreeLockUtils.doWithTreeLock(
- NameIdentifier.of(NamespaceUtil.ofPolicy(metalake).levels()),
- LockType.WRITE,
- () -> {
- try {
- List<PolicyEntity> updatedPolicies =
- entityStore
- .relationOperations()
- .updateEntityRelations(
-
SupportsRelationOperations.Type.POLICY_METADATA_OBJECT_REL,
- entityIdent,
- entityType,
- policiesToAddIdent,
- policiesToRemoveIdent);
- return
updatedPolicies.stream().map(PolicyEntity::name).toArray(String[]::new);
- } catch (NoSuchEntityException e) {
- throw new NoSuchMetadataObjectException(
- e,
- "Failed to associate policies for metadata object %s
due to not found",
- metadataObject);
- } catch (EntityAlreadyExistsException e) {
- throw new PolicyAlreadyAssociatedException(
- e,
- "Failed to associate policies for metadata object due
to some policies %s already "
- + "associated to the metadata object %s",
- Arrays.toString(policiesToAdd),
- metadataObject);
- } catch (IOException e) {
- LOG.error(
- "Failed to associate policies for metadata object {}",
metadataObject, e);
- throw new RuntimeException(e);
- }
- }));
- }
-
- @Override
- public PolicyEntity getPolicyForMetadataObject(
- String metalake, MetadataObject metadataObject, String policyName) {
- try {
- return Arrays.stream(listPolicyInfosForMetadataObject(metalake,
metadataObject))
- .filter(policy -> policy.name().equals(policyName))
- .findFirst()
- .orElseThrow(
- () ->
- new NoSuchPolicyException(
- "Policy %s does not exist for metadata object %s",
- policyName, metadataObject));
- } catch (NoSuchMetadataObjectException e) {
- throw new NoSuchMetadataObjectException(
- e, "Failed to get policy for metadata object %s due to not found",
metadataObject);
- }
- }
-
- private PolicyEntity[] listDirectPoliciesForMetadataObject(
- String metalake, MetadataObject metadataObject, boolean inherited) {
- NameIdentifier entityIdent = MetadataObjectUtil.toEntityIdent(metalake,
metadataObject);
- Entity.EntityType entityType =
MetadataObjectUtil.toEntityType(metadataObject);
- return TreeLockUtils.doWithTreeLock(
- entityIdent,
- LockType.READ,
- () -> {
- try {
- return entityStore
- .relationOperations()
- .listEntitiesByRelation(
- SupportsRelationOperations.Type.POLICY_METADATA_OBJECT_REL,
- entityIdent,
- entityType,
- true /* allFields */)
- .stream()
- .map(entity -> ((PolicyEntity)
entity).copyWithInherited(inherited))
- .toArray(PolicyEntity[]::new);
- } catch (NoSuchEntityException e) {
- throw new NoSuchMetadataObjectException(
- e,
- "Failed to list policies for metadata object %s due to not
found",
- metadataObject);
- } catch (IOException e) {
- LOG.error("Failed to list policies for metadata object {}",
metadataObject, e);
- throw new RuntimeException(e);
- }
- });
+ return objectPolicyResolver.resolve(metalake, metadataObject);
Review Comment:
[Question] This read path now runs with no tree lock at all. On `main` the
direct half of this method went through
`TreeLockUtils.doWithTreeLock(entityIdent, LockType.READ, ...)` in
`listDirectPoliciesForMetadataObject` (main's `PolicyManager.java:420-424`);
the resolver half was already unlocked. After this change the whole listing is
unlocked, and `ObjectPolicyResolver.resolve` -> `EffectiveTagResolver.resolve`
issues one relation query per ancestor in a loop
(`EffectiveTagResolver.java:71-90`) plus a batch policy-tag lookup, so a
concurrent drop or rename of the object, an ancestor, or a tag can interleave
mid-walk and produce a partially-stale policy set.
If that is an accepted trade (reads are best-effort and the old lock only
ever covered the direct rows), no change needed - a one-line comment here
saying so would stop it being re-introduced by accident. If not, wrapping the
call in a READ tree lock on `MetadataObjectUtil.toEntityIdent(metalake,
metadataObject)` restores the previous guarantee.
Verified by: read the removed `listDirectPoliciesForMetadataObject` in `git
show
origin/main:core/src/main/java/org/apache/gravitino/policy/PolicyManager.java`
and confirmed neither `ObjectPolicyResolver` nor `EffectiveTagResolver` takes
any lock.
##########
maintenance/optimizer/src/test/java/org/apache/gravitino/maintenance/optimizer/integration/test/AbstractGravitinoOptimizerEnvIT.java:
##########
@@ -115,17 +116,24 @@ protected void createPolicy(String policyName,
Map<String, Object> rules, String
GravitinoStrategy.JOB_TEMPLATE_NAME_KEY,
"template-name"));
metalakeClient.createPolicy(policyName, "custom", "comment", true,
content);
+ String tagName = policyTagName(policyName);
+ metalakeClient.createTag(tagName, "comment", Map.of());
+ metalakeClient.addPolicyForTag(tagName, policyName,
AllValuesSelector.get());
Review Comment:
[Nit] `createPolicy` now has a hidden side effect - it also creates a tag
named `tag_<policyName>` and binds the policy to it - while the callers are
still named `associatePoliciesToTable` / `associatePoliciesToSchema` even
though they now call `supportsTags().associateTags(...)`. Two small
consequences worth considering: creating a policy without a tag is no longer
possible from this base class (so a case like "policy exists but no tag
assigned -> no strategies" can't be expressed), and a second `createPolicy`
call with an already-used name would now fail on `TagAlreadyExistsException`
rather than on the policy. Current callers all use distinct names
(`RecommenderIT` lines 102/113/181, `GravitinoStrategyIT` lines 50/63/64), so
nothing breaks today; splitting the tag creation into its own helper and
renaming these two methods to say "tag" would keep the ITs readable.
Verified by: read this file plus both IT subclasses on this branch.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
