jerryshao commented on code in PR #14326:
URL: https://github.com/apache/gravitino/pull/14326#discussion_r4438556119
##########
core/src/main/java/org/apache/gravitino/tag/TagManager.java:
##########
@@ -120,6 +124,7 @@ public Tag createTag(
Map<String, String> properties,
TagValueConstraint valueConstraint)
throws TagAlreadyExistsException {
+ HiddenPolicyTag.checkVisible(name);
Review Comment:
`checkVisible(String)` runs before the row is loaded, so any pre-upgrade tag
whose name starts with `reserved_` in any case stays in `listTags`
(`hidden=false`), but get/alter/rename/delete throw `IllegalArgumentException`,
and the user cannot rename it off the prefix (same at L158, L165, L207, ...).
`uk_mi_tn_del` is case-sensitive (`utf8mb4_bin` on MySQL/H2, varchar on
PostgreSQL), so `Reserved_1` does not collide with `reserved_1`. Meanwhile, an
existing `reserved_{policyId}` row makes `insertTag` fail, and that failure
surfaces as `PolicyAlreadyExistsException` (`PolicyManager.java:244-246` plus
the duplicate-key rewrite in `PolicyMetaService.insertPolicy`).
Please apply the prefix check only to create and to the target name of a
rename, protect stored rows by `hidden == true`, and return a tag-collision
error that names the tag. A test for a pre-upgrade `reserved_*` tag in both
casings would help.
##########
core/src/main/java/org/apache/gravitino/storage/relational/service/PolicyMetaService.java:
##########
@@ -499,6 +535,26 @@ private void updatePolicyRootWithVersion(
() -> policyWriteFailure(identifier, oldPolicyPO));
}
+ private void deleteHiddenTagWithoutCommit(PolicyPO policy) {
+ TagPO tag =
+ SessionUtils.getWithoutCommit(
+ TagMetaMapper.class,
+ mapper ->
+ mapper.selectTagMetaByMetalakeIdAndNameForUpdate(
+ policy.getMetalakeId(),
HiddenPolicyTag.name(policy.getPolicyId())));
+ if (tag == null
+ || !tag.isHidden()
+ ||
!HiddenPolicyTag.properties(policy.getPolicyId()).equals(tag.getProperties())) {
Review Comment:
This returns without soft-deleting when the stored properties text is not
byte-for-byte `HiddenPolicyTag.properties()`. `softDeleteByPolicyId` still
removes the policy link afterwards, so the tag and its object rows stay at
`deleted_at=0` and GC never purges them. Please match on the parsed property
(or `hidden` plus the canonical name), require the soft-delete to update
exactly one row, and invalidate the tag cache entry;
`RelationalEntityStore.delete` only invalidates the policy id.
##########
core/src/main/java/org/apache/gravitino/policy/PolicyManager.java:
##########
@@ -73,6 +78,100 @@ public PolicyManager(IdGenerator idGenerator, EntityStore
entityStore) {
this.objectPolicyResolver = new ObjectPolicyResolver(entityStore);
}
+ /**
+ * Applies a policy to an object through its hidden tag using the entity
store.
+ *
+ * <p>This internal operation is idempotent. Migration callers can group it
with legacy cleanup in
+ * their storage transaction. It is not exposed through the policy
dispatcher or client APIs.
+ *
+ * @param metalake The policy and object's metalake.
+ * @param policyName The policy name.
+ * @param object The metadata object to associate.
+ * @param auditInfo The audit information to preserve on newly created
relations.
+ * @return The policy's hidden tag ID.
+ * @throws IOException If an entity or relation cannot be persisted.
+ */
+ public long addPolicyToObject(
Review Comment:
`addPolicyToObject` commits the hidden tag, the policy-tag link, and the
object assignment separately. Two concurrent callers can both see no policy
link; `insertIfAbsent` then throws `EntityAlreadyExistsException`
(`PolicyTagRelService.java:314`), and this call never associates the object.
Please treat an existing link to this policy with no selector as success, the
same as the `policies.isEmpty()` else branch.
##########
core/src/main/java/org/apache/gravitino/storage/relational/mapper/provider/base/TagMetaBaseSQLProvider.java:
##########
@@ -33,7 +33,7 @@ public String listTagPOsByMetalake(@Param("metalakeName")
String metalakeName) {
+ " tm.metalake_id as metalakeId,"
+ " tm.tag_comment as comment,"
+ " tm.properties as properties,"
- + " tm.allowed_values as allowedValues,"
+ + " tm.allowed_values as allowedValues, tm.hidden as hidden,"
Review Comment:
Old servers select and update `tag_meta` without the `hidden` column, so a
`hidden=true` row looks like an ordinary tag to them and can be renamed or
deleted during a mixed rollout. `DEFAULT FALSE` keeps old inserts safe. Please
document that every server must be upgraded together with the schema before any
policy is created.
##########
scripts/h2/upgrade-1.3.0-to-2.0.0-h2.sql:
##########
@@ -227,5 +227,8 @@ ALTER TABLE `fileset_meta`
ALTER TABLE `policy_meta`
ADD COLUMN IF NOT EXISTS `occ_version` INT UNSIGNED NOT NULL DEFAULT 1
COMMENT 'policy optimistic concurrency version' AFTER `last_version`;
+
Review Comment:
nit: Please quote `tag_meta` and `hidden` and add the same `COMMENT` as
`schema-2.0.0-h2.sql`, matching the statements above this line.
##########
core/src/main/java/org/apache/gravitino/storage/relational/service/PolicyMetaService.java:
##########
@@ -434,6 +446,30 @@ private void insertPolicyVersionIfAllocated(PolicyPO
oldPolicyPO, PolicyPO newPo
mapper ->
mapper.insertPolicyVersion(newPolicyPO.getPolicyVersionPO()));
}
+ private void createHiddenTagWithoutCommit(PolicyEntity policy, PolicyPO
policyPO) {
+ TagEntity tag = HiddenPolicyTag.create(policy,
RandomIdGenerator.INSTANCE.nextId());
Review Comment:
nit: Please pass the server `IdGenerator` into hidden-tag creation instead
of `RandomIdGenerator.INSTANCE`, so tag ids come from the same generator as
policy ids.
--
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]
