yuqi1129 commented on code in PR #12855:
URL: https://github.com/apache/gravitino/pull/12855#discussion_r3922608644
##########
core/src/main/java/org/apache/gravitino/listener/TagEventDispatcher.java:
##########
@@ -238,6 +249,71 @@ public MetadataObject[] listMetadataObjectsForTag(String
metalake, String name,
}
}
+ @Override
+ public RelationalEntity<?>[] listPolicyAssociationsForTag(String metalake,
String name) {
+ return dispatcher.listPolicyAssociationsForTag(metalake, name);
+ }
+
+ @Override
+ public void addPolicyForTag(
+ String metalake, String tagName, String policyName,
PolicyAssociationSelector selector) {
+ String user = PrincipalUtils.getCurrentUserName();
+ PolicyTagAssociationInfo previousAssociation;
+ try {
+ previousAssociation = findPolicyAssociation(metalake, tagName,
policyName);
Review Comment:
Blocker: this snapshot is not atomic with the mutation.
`listPolicyAssociationsForTag` takes and releases a read lock, then add/remove
later takes a separate write lock. For example, R1 can read association A, R2
can remove A, and then the idempotent remove in R1 succeeds while its event
incorrectly claims that R1 removed A. The same gap exists across server nodes,
so the storage OCC does not make this event snapshot accurate. This also loads
every policy relation for the tag and filters in Java, making each point
mutation O(N). A better design is for `TagManager` / `PolicyTagRelService` to
return a domain-level mutation result containing the actual
previous/resulting/removed association captured in the same transaction; the
event dispatcher should only translate that result into events and should not
know about `RelationalEntity` or selector JSON. If a pre-event must contain the
previous value, use a point lookup with a version token and require the write
to CAS that version afte
r the listener runs. Please add a deterministic concurrent remove/remove or
remove/add regression test that asserts the event matches the mutation
committed by that request.
##########
core/src/main/java/org/apache/gravitino/listener/TagEventDispatcher.java:
##########
@@ -238,6 +249,71 @@ public MetadataObject[] listMetadataObjectsForTag(String
metalake, String name,
}
}
+ @Override
+ public RelationalEntity<?>[] listPolicyAssociationsForTag(String metalake,
String name) {
+ return dispatcher.listPolicyAssociationsForTag(metalake, name);
+ }
+
+ @Override
+ public void addPolicyForTag(
+ String metalake, String tagName, String policyName,
PolicyAssociationSelector selector) {
+ String user = PrincipalUtils.getCurrentUserName();
+ PolicyTagAssociationInfo previousAssociation;
+ try {
+ previousAssociation = findPolicyAssociation(metalake, tagName,
policyName);
+ } catch (Exception e) {
+ eventBus.dispatchEvent(
Review Comment:
At this point the snapshot operation has already failed, so emitting a
PreEvent is both out of order and unsafe. `EventBus` permits a pre-event
listener to throw `ForbiddenException`; if that happens here, the listener
exception masks the original storage exception, the FailureEvent is never
emitted, and a real server error can be reported as a denial. Please emit an
intent-only PreEvent before any business read/write, then execute
snapshot/mutation inside the normal try/catch and preserve the original
exception in the FailureEvent. Add a test where the snapshot throws and the pre
listener also throws, asserting that the primary failure is not replaced and
that the documented event sequence is preserved.
##########
core/src/main/java/org/apache/gravitino/audit/v2/CompatibilityUtils.java:
##########
@@ -75,6 +75,8 @@ public class CompatibilityUtils {
Operation.LIST_TAGS_INFO_FOR_METADATA_OBJECT)
.put(OperationType.LIST_METADATA_OBJECTS_FOR_TAG,
Operation.LIST_METADATA_OBJECTS_FOR_TAG)
.put(OperationType.LIST_TAGS_INFO, Operation.LIST_TAGS_INFO)
+ .put(OperationType.ADD_POLICY_FOR_TAG, Operation.ADD_POLICY_FOR_TAG)
Review Comment:
The operation mapping is correct now, but the formatted audit record still
loses the relation target. Both formatters serialize the event identifier,
which is only the tag here, and these events return empty `customInfo`; the
audit log therefore cannot show which policy or selector was added or removed.
For a governance relation change, please include at least `policyName` and,
where appropriate, previous/requested/resulting selector information in the v2
audit payload, define the intended v1 limitation, and test the actual formatter
output rather than only this enum mapping.
##########
core/src/main/java/org/apache/gravitino/listener/api/event/AddPolicyForTagEvent.java:
##########
@@ -0,0 +1,90 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.gravitino.listener.api.event;
+
+import java.util.Optional;
+import javax.annotation.Nullable;
+import org.apache.gravitino.annotation.DeveloperApi;
+import org.apache.gravitino.listener.api.info.PolicyTagAssociationInfo;
+import org.apache.gravitino.policy.PolicyAssociationSelector;
+import org.apache.gravitino.utils.NameIdentifierUtil;
+
+/** Represents an event triggered after successfully adding a policy to a tag.
*/
+@DeveloperApi
+public final class AddPolicyForTagEvent extends TagEvent {
+ @Nullable private final PolicyTagAssociationInfo previousAssociation;
Review Comment:
Please settle the public mutation contract before publishing this
`@DeveloperApi` event shape. The current `TagOperations` contract says add
never replaces an existing relation and `TagManager` throws
`PolicyAlreadyAssociatedException`, while the policy-on-tag design document
defines PUT as create-or-replace and idempotent. Under the current runtime,
`previousAssociation` can never be present on a successful add event, so this
success field describes an impossible state. If the operation is create-only,
remove the successful previous snapshot and its extra read. If it is the
intended PUT/upsert, implement an atomic selector update and use a
SET/ASSOCIATE-style operation name rather than ADD. Tests should cover a
missing relation, the same selector repeated, and an existing relation with a
different selector according to the chosen contract.
--
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]
