jerryshao commented on code in PR #13355:
URL: https://github.com/apache/gravitino/pull/13355#discussion_r4060222546
##########
core/src/main/java/org/apache/gravitino/policy/PolicyManager.java:
##########
@@ -302,150 +252,8 @@ 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);
- }
- });
+ // Tag-derived policy reads are best-effort across the object and its
ancestors.
Review Comment:
[Nit] The comment records the conclusion but not the thing it is meant to
protect: that this path now deliberately takes **no tree lock**. Before this PR
the direct half of the listing ran under
`TreeLockUtils.doWithTreeLock(entityIdent, LockType.READ, ...)` (base
`9f18a9e`,
`core/src/main/java/org/apache/gravitino/policy/PolicyManager.java:420-424`);
the whole listing is now lock-free, and `EffectiveTagResolver.resolve` issues
one relation query per ancestor in a loop
(`core/src/main/java/org/apache/gravitino/tag/EffectiveTagResolver.java:74-89`).
As written, "best-effort" does not tell a future reader that the missing lock
is intentional, which was the point of the comment.
Suggest: `// Resolved without a tree lock: tag-derived policy reads are
best-effort across the object and its ancestors.`
Verified by: read the base and head versions of `PolicyManager` and
`EffectiveTagResolver` in a local clone at `1d8ff2b`.
--
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]
