[#12775] feat(core): Add OCC for tag metadata - #12781
Open
yuqi1129 wants to merge 4 commits into
Open
Conversation
Contributor
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Pull request overview
Adds optimistic concurrency control (OCC) to tag metadata mutations to prevent lost updates and stale deletes, and ensures tag versions remain monotonic during overwrites.
Changes:
- Introduce version-checked (stable-ID + expected-version) update/delete paths for tags and dependent relationship cleanup.
- Ensure tag overwrite increments the stored OCC sequence (monotonic current/last versions).
- Add row-level locking for metalake/tag roots during tag creation and tag-assignment changes, plus new OCC-focused tests.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| core/src/test/java/org/apache/gravitino/storage/relational/service/TestTagMetaService.java | Adds tests covering monotonic versions and optimistic-lock failure behavior for tag alter/delete. |
| core/src/main/java/org/apache/gravitino/storage/relational/utils/POConverters.java | Updates tag PO version bump logic to increment from current version. |
| core/src/main/java/org/apache/gravitino/storage/relational/service/TagMetaService.java | Applies OCC + locking for create/update/delete and tag-assignment flows; adds relationship cleanup by tag ID. |
| core/src/main/java/org/apache/gravitino/storage/relational/mapper/provider/postgresql/TagMetadataObjectRelPostgreSQLProvider.java | Adds Postgres-specific SQL for soft-deleting tag-object relations by tag ID. |
| core/src/main/java/org/apache/gravitino/storage/relational/mapper/provider/postgresql/TagMetaPostgreSQLProvider.java | Switches tag soft-delete to (tag_id, current_version) and enforces monotonic overwrite versioning in Postgres. |
| core/src/main/java/org/apache/gravitino/storage/relational/mapper/provider/base/TagMetadataObjectRelBaseSQLProvider.java | Adds base SQL for soft-deleting tag-object relations by tag ID. |
| core/src/main/java/org/apache/gravitino/storage/relational/mapper/provider/base/TagMetaBaseSQLProvider.java | Updates OCC semantics for update/delete SQL and includes allowed_values in select projections. |
| core/src/main/java/org/apache/gravitino/storage/relational/mapper/provider/base/PolicyTagRelBaseSQLProvider.java | Adds SQL to soft-delete policy-tag relations by tag ID during tag deletion. |
| core/src/main/java/org/apache/gravitino/storage/relational/mapper/TagMetadataObjectRelSQLProviderFactory.java | Exposes provider factory method for deleting tag-object relations by tag ID. |
| core/src/main/java/org/apache/gravitino/storage/relational/mapper/TagMetadataObjectRelMapper.java | Adds mapper API to soft-delete tag-object relations by tag ID. |
| core/src/main/java/org/apache/gravitino/storage/relational/mapper/TagMetaSQLProviderFactory.java | Renames/rewires soft-delete SQL to version-checked delete by (tag_id, current_version). |
| core/src/main/java/org/apache/gravitino/storage/relational/mapper/TagMetaMapper.java | Adds mapper API for version-checked soft delete by tag ID + observed version. |
| core/src/main/java/org/apache/gravitino/storage/relational/mapper/PolicyTagRelSQLProviderFactory.java | Exposes provider factory method for deleting policy-tag relations by tag ID. |
| core/src/main/java/org/apache/gravitino/storage/relational/mapper/PolicyTagRelMapper.java | Adds mapper API to soft-delete policy-tag relations by tag ID. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Code Coverage Report
Files
|
jerryshao
reviewed
Sep 1, 2026
jerryshao
left a comment
jerryshao
left a comment
Contributor
There was a problem hiding this comment.
Automated code review (Claude Code) — 10 findings below, most severe first. This PR is notably cleaner than the companion Model-OCC PR (#12649): I specifically checked for the same TOCTOU alias-race bug class found there and it's absent here — this PR correctly uses the shared OccWriteSupport helper (from #12639) for its single-entity locking paths. The remaining findings are an undocumented cascade-delete safety gap, several missing tests for new failure branches, one real reuse gap (a hand-rolled duplicate of OccWriteSupport), a CLAUDE.md convention violation, and an observability regression.
- lockTagsForAssignment goes through OccWriteSupport.lockParentForChildWrite instead of repeating the null-and-identity check inline, so the three tag OCC paths classify a vanished row the same way. - Documented why the assignment change runs in one transaction, why the metalake row is locked in share mode without comparing its version, and why the tag rows are locked in ID order. - lockMetalakeForTagCreate is private; the deleteTag overload that takes an observed row is marked @VisibleForTesting, which is the only reason it is not private. - Added coverage for parent-metalake fencing on create and for allowed values surviving an overwrite and an alter, which the widened select lists now carry. Claude-Session: https://claude.ai/code/session_01H8XCY74Bmjbcr618EKxNbA
- Tag rows are locked by one statement (listTagPOsByTagIdsForUpdate, ordered by tag ID) instead of one SELECT ... FOR UPDATE per tag, so an assignment change touching N tags costs one round trip while keeping the ID lock order that avoids deadlocks. - PolicyTagRelService.lockTag routes through the same helper, so the three copies of "lock a tag row and check its identity" are now one. - The package-private deleteTag overload moved below the public methods, as the member ordering in CLAUDE.md asks. - Documented that the version-checked delete has to stay first in deleteTag and why the cascade cleanups that follow are not row-count checked. - TagManager logs a concurrent alter or delete with the tag and metalake before rethrowing, which the old catch (IOException) used to do. - Added coverage for the cascade cleanup a tag delete performs (assignments, policy-tag relations, policies applied to the tag, its owner relation and the securable objects that name it) and for a delete of an already deleted tag being reported as missing rather than as a conflict. Claude-Session: https://claude.ai/code/session_01H8XCY74Bmjbcr618EKxNbA
yuqi1129
force-pushed
the
feat/12775-occ-tag
branch
from
September 2, 2026 13:57
2c7f735 to
d3eafc8
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
Why are the changes needed?
Concurrent tag writes could otherwise cause lost updates, stale deletions, ID/version resets, or partial relationship cleanup.
Fix: #12775
Does this PR introduce any user-facing change?
No API or configuration change. Concurrent stale writes now fail deterministically; a tag deleted or renamed by a competing transaction preserves the existing idempotent delete result.
How was this patch tested?
./gradlew :core:spotlessApply :core:compileTestJava :core:javadoc./gradlew :core:test --tests org.apache.gravitino.storage.relational.service.TestTagMetaService --tests org.apache.gravitino.storage.relational.service.TestPolicyTagRelService --tests org.apache.gravitino.tag.TestTagManager -PskipDockerTests=falseTestTagMetaServiceran against H2, MySQL, and PostgreSQL.