jerryshao commented on PR #14326: URL: https://github.com/apache/gravitino/pull/14326#issuecomment-6680996019
Verdict: no blocking issues Reviewed apache/gravitino#14326 at head cc749f80921e9023006afb51e128d1ce4d3e02c9 (matches requested head-sha cc749f8). I checked out the head, diffed against the merge base 5460487c, and read the new `HiddenPolicyTag`, `PolicyManager.addPolicyToObject`, the `PolicyMetaService` create/delete changes, the audit pass-through in `RelationUpdate`/`PolicyTagRelService`/`TagMetaService`/`JDBCBackend`, and every REST guard (`TagOperations`, `MetadataObjectTagOperations`, `PolicyOperations`, role/owner/permission/bulk operations). Correct: the hidden tag is created and deleted in the same transaction as the policy; its name is keyed on the policy id so it survives renames; REST list/get/alter/delete/associate paths and role/owner/permission endpoints reject or filter reserved names; `addPolicyToObject` is not exposed through the dispatcher. Gaps: no migration or backfill for policies that already exist, a broad reserved-prefix that can hide pre-existing user tags, and nested-transaction behavior that I could confirm only by reading. I did not run the test suite. Five inline notes follow. --- **Inline findings** **`core/src/main/java/org/apache/gravitino/policy/PolicyManager.java:111`** [Question] Policies created before this change have no hidden tag, and I found no backfill or upgrade script (grepped scripts/ and the diff). For such a policy, `entityStore.get(tagIdent, TAG, ...)` on the next line throws `NoSuchEntityException`, so `addPolicyToObject` fails for every pre-existing policy. Is a backfill planned, or should this method create the hidden tag lazily when it is missing? The overwrite path in `PolicyMetaService.insertPolicyWithoutCommit` also does not create one for an existing policy. Verified by reading: the tag is created only in the two `!overwritten`/new-row branches. --- **`core/src/main/java/org/apache/gravitino/policy/HiddenPolicyTag.java:72`** [Important] `isReservedName` is a case-insensitive prefix match on `gravitino_hidden_`, and the REST layer now hides and rejects any tag with that prefix. Existing user tags with such names (for example `Gravitino_Hidden_pii`) become invisible: they are dropped from list, and get, alter, delete and associate return 404 (`checkVisibleTag`). Their metadata-object assignments are also filtered out, so they cannot be cleaned up through the API. Failing scenario: upgrade a cluster that has a tag named `gravitino_hidden_x`; it is silently unreachable. Suggested fix: match the exact shape `gravitino_hidden_<digits>` (case-sensitive, or at least the properties marker `gravitino.internal.policy-id`) and/or add an upgrade check that reports colliding tag names. This also shrinks the set of names users are forbidden to create. --- **`core/src/main/java/org/apache/gravitino/storage/relational/service/PolicyMetaService.java:453`** [Question] `createHiddenTagWithoutCommit` calls `TagMetaService.insertTag`, which opens its own `doMultipleWithCommit` and re-locks the metalake while already inside the outer policy transaction. This works only if `SqlSessions` counts nested sessions and defers commit to the outermost one. I read `doMultipleWithCommit` (it calls `getSqlSession()` then `commitAndCloseSqlSession()`), which suggests it is re-entrant, but please confirm there is a test where the policy insert fails after the tag insert and both rows roll back (no orphan hidden tag). Also, an `IOException` is wrapped in `UncheckedIOException` and unwrapped in `insertPolicy`; a failing tag insert of type `EntityAlreadyExistsException` for the TAG would be mapped by `checkSQLException` using the POLICY type, which may give a misleading message. --- **`core/src/main/java/org/apache/gravitino/storage/relational/service/PolicyMetaService.java:544`** [Nit] `deleteHiddenTagWithoutCommit` hand-rolls tag soft-delete (tag row, tag-object rels, owner, securable objects, change log) instead of reusing `TagMetaService`'s delete logic, so any later change to tag deletion (new dependent tables) must be mirrored here. Consider extracting a shared `...WithoutCommit` helper. It also does not remove `policy_tag_rel` rows for the tag explicitly, but `softDeleteByPolicyId` later in the same transaction covers that. --- **`server/src/main/java/org/apache/gravitino/server/web/rest/PolicyOperations.java:357`** [Nit] Filtering on name prefix is applied after the policy-to-tag association list is fetched, so the hidden-tag edge is dropped only at the REST layer. Other consumers of `listTagAssociationsForPolicy` (Java client through other dispatchers, `ObjectPolicyResolver`) still see it. If that is intended (the hidden tag is how policies bind), a short comment saying so would help; otherwise consider filtering in the dispatcher. -- 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]
