jerryshao commented on code in PR #12781:
URL: https://github.com/apache/gravitino/pull/12781#discussion_r3903581999
##########
core/src/main/java/org/apache/gravitino/storage/relational/service/TagMetaService.java:
##########
@@ -156,27 +174,47 @@ public <E extends Entity & HasIdentifier> TagEntity
updateTag(
@Monitored(metricsSource = GRAVITINO_RELATIONAL_STORE_METRIC_NAME,
baseMetricName = "deleteTag")
public boolean deleteTag(NameIdentifier identifier) {
- String metalakeName = identifier.namespace().level(0);
- int[] tagDeletedCount = new int[] {0};
- int[] tagMetadataObjectRelDeletedCount = new int[] {0};
+ TagPO tagPO;
+ try {
+ tagPO = getTagPOByMetalakeAndName(identifier.namespace().level(0),
identifier.name());
+ } catch (NoSuchEntityException e) {
+ return false;
+ }
+ return deleteTag(identifier, tagPO);
+ }
+
+ boolean deleteTag(NameIdentifier identifier, TagPO tagPO) {
+ long tagId = tagPO.getTagId();
SessionUtils.doMultipleWithCommit(
Review Comment:
**altitude**: `deleteTag`'s five cascade cleanup calls (tag-metadata-object
rel, policy-tag rel, policy-metadata-object rel, owner rel, securable-object
rel) are fired via plain `SessionUtils.doWithoutCommit` with no row-count
verification, relying entirely on the undocumented invariant that
`deleteTagWithVersion` runs first in this `doMultipleWithCommit` varargs list —
unlike sibling services (e.g. `SchemaMetaService`), which route cascade cleanup
through `OccWriteSupport.deleteChildrenWithVersions` to assert the affected row
count.
Correctness of the whole cascade depends on `deleteTagWithVersion` (line
190) staying the first element of the varargs list, with nothing in the code or
a comment flagging that requirement. If a future refactor reorders the
suppliers, adds an early cascade step before the version-CAS, or copies this
pattern into a context where cleanup runs in a separately-committed session, a
stale/losing delete could silently remove 0 child rows (or race) with no error
surfaced — unlike the `deleteChildrenWithVersions`-based services elsewhere,
which would explicitly throw a concurrent-child-modification exception instead
of silently succeeding on a partial cleanup.
##########
core/src/main/java/org/apache/gravitino/storage/relational/service/TagMetaService.java:
##########
@@ -156,27 +174,47 @@ public <E extends Entity & HasIdentifier> TagEntity
updateTag(
@Monitored(metricsSource = GRAVITINO_RELATIONAL_STORE_METRIC_NAME,
baseMetricName = "deleteTag")
public boolean deleteTag(NameIdentifier identifier) {
- String metalakeName = identifier.namespace().level(0);
- int[] tagDeletedCount = new int[] {0};
- int[] tagMetadataObjectRelDeletedCount = new int[] {0};
+ TagPO tagPO;
+ try {
+ tagPO = getTagPOByMetalakeAndName(identifier.namespace().level(0),
identifier.name());
+ } catch (NoSuchEntityException e) {
+ return false;
+ }
+ return deleteTag(identifier, tagPO);
+ }
+
+ boolean deleteTag(NameIdentifier identifier, TagPO tagPO) {
+ long tagId = tagPO.getTagId();
SessionUtils.doMultipleWithCommit(
+ () -> deleteTagWithVersion(identifier, tagPO),
() ->
Review Comment:
**test-coverage**: The four new cascade cleanup calls this PR wires into
`deleteTag` — for policy-tag relations, policy-metadata-object relations, owner
relations, and securable-object relations — have no test assertions anywhere in
the diff.
CLAUDE.md: "Write unit tests for ALL new logic. NO tests = NO merge." The
new test `testStaleTagDeleteRollsBackRelationshipCleanup` only asserts
`countActiveTagRel` (the one cascade, tag-metadata-object rel, that already
existed pre-PR). A wiring mistake in any of the four newly-added cascade calls
— wrong mapper method, wrong id/type argument, or a missing rollback under the
OCC-conflict path — would not be caught by CI, and combined with the missing
row-count check (see the companion comment on this same method), a bug here
would fail silently rather than loudly.
##########
core/src/main/java/org/apache/gravitino/storage/relational/service/TagMetaService.java:
##########
@@ -625,6 +678,66 @@ private static void validateAllowedValue(TagPO tagPO,
TagValue tagValue)
Arrays.toString(allowedValues));
}
+ void lockMetalakeForTagCreate(MetalakePO observedMetalakePO) {
+ OccWriteSupport.lockParentForChildWrite(
+ observedMetalakePO.getMetalakeName(),
+ Entity.EntityType.METALAKE,
+ () ->
+ SessionUtils.getWithoutCommit(
+ MetalakeMetaMapper.class,
+ mapper ->
+
mapper.selectMetalakeMetaByIdForShare(observedMetalakePO.getMetalakeId())),
+ null,
+ current -> Objects.equals(current.getMetalakeName(),
observedMetalakePO.getMetalakeName()));
+ }
+
+ private void deleteTagWithVersion(NameIdentifier identifier, TagPO
observedTagPO) {
+ OccWriteSupport.deleteWithVersion(
+ () ->
+ SessionUtils.getWithoutCommit(
+ TagMetaMapper.class,
+ mapper ->
+ mapper.softDeleteTagMetaByIdAndVersion(
+ observedTagPO.getTagId(),
observedTagPO.getCurrentVersion())),
+ () -> tagWriteFailure(identifier, observedTagPO));
+ }
+
+ private RuntimeException tagWriteFailure(NameIdentifier identifier, TagPO
observedTagPO) {
+ return OccWriteSupport.writeFailure(
+ identifier,
+ Entity.EntityType.TAG,
+ () ->
+ SessionUtils.getWithoutCommit(
+ TagMetaMapper.class,
+ mapper ->
mapper.selectTagByTagIdForUpdate(observedTagPO.getTagId())),
+ null,
+ current ->
+ Objects.equals(current.getTagName(), observedTagPO.getTagName())
+ && Objects.equals(current.getMetalakeId(),
observedTagPO.getMetalakeId()));
+ }
+
+ private List<TagPO> lockTagsForAssignment(List<TagPO> observedTagPOs) {
Review Comment:
**reuse**: `lockTagsForAssignment` hand-rolls the identical
"select-for-update, then verify null/name/metalakeId identity, then throw
`NoSuchEntityException`" logic that `OccWriteSupport.lockParentForChildWrite`
already provides — and that this very class calls correctly ~30 lines earlier
in `lockMetalakeForTagCreate`.
The loop body reimplements exactly what
`OccWriteSupport.lockParentForChildWrite` (already used elsewhere in this same
PR and by `CatalogMetaService`/`SchemaMetaService`) does per item. A future fix
to the shared classification logic (e.g. an exception-message format change, or
adding a check) will silently miss this call site since it isn't routed through
the helper, reintroducing the drift `OccWriteSupport` (added by #12639) was
built specifically to eliminate. The identical tagName/metalakeId identity
predicate is also separately duplicated in `tagWriteFailure` (line 705-717) in
the same file.
---
**altitude** (same location): The codebase now has three independent
implementations of "lock a tag row and verify its identity is unchanged":
`tagWriteFailure`'s predicate, this new `lockTagsForAssignment`, and the
pre-existing `PolicyTagRelService.lockTag` (untouched by this PR) — despite
this PR actively touching tag-locking logic and having the natural opportunity
to consolidate. A future change to the identity-check semantics now has to be
found and updated in three places across two files; it's easy to update two and
miss the third.
---
**efficiency** (same location): This also locks tags one row at a time in a
loop (`selectTagByTagIdForUpdate` per tag) instead of a single batched `SELECT
... WHERE tag_id IN (...) FOR UPDATE`, on top of an earlier batch SELECT
(`getTagPOsByMetalakeAndNames`) whose result is discarded and immediately
re-fetched row-by-row here. For a metadata object tagged with 10 tags, that's
10+ extra DB round trips serialized inside one transaction. A batched, `FOR
UPDATE` variant of the existing `listTagPOsByTagIds`-style query would collapse
this to one round trip while preserving the tagId-ascending lock order already
used to avoid deadlocks.
---
**test-coverage** (same location): This method's concurrent-modification
branch (thrown when a tag being associated was renamed or deleted between the
initial name lookup and the locking re-read) also has no dedicated test — none
of the new tests in `TestTagMetaService.java`'s diff hunk exercise a tag
renamed/deleted mid-`associateTagValuesWithMetadataObject`.
##########
core/src/main/java/org/apache/gravitino/storage/relational/service/TagMetaService.java:
##########
@@ -143,7 +161,7 @@ public <E extends Entity & HasIdentifier> TagEntity
updateTag(
POConverters.updateTagPOWithVersion(tagPO,
updatedTagEntity), tagPO));
if (result == null || result == 0) {
- throw new IOException("Failed to update the entity: " + identifier);
+ throw tagWriteFailure(identifier, tagPO);
Review Comment:
**correctness / observability**: `updateTag`'s CAS-failure path now throws
`tagWriteFailure`'s unchecked exception types
(`NoSuchEntityException`/`OptimisticLockException`) instead of the old checked
`IOException`. This makes `TagManager.alterTag`'s `catch (IOException ioe)`
block (server/core
`core/src/main/java/org/apache/gravitino/tag/TagManager.java:202-205`, which
logs at ERROR with tag/metalake context before rethrowing) dead code for the
concurrent-alter-conflict case, since that file isn't touched by this diff and
the new exception type skips straight past it to the generic REST-layer
`ExceptionHandlers` (which does correctly map `OptimisticLockException` to an
HTTP conflict, so end users aren't broken).
The operator-facing ERROR log with tag/metalake context is silently bypassed
in favor of a more generic WARN logged elsewhere — an observability regression
for anyone relying on `TagManager`'s logs to diagnose concurrent-write
conflicts, and the now-unreachable `catch (IOException ioe)` block in
`TagManager.alterTag` is worth a cleanup pass.
##########
core/src/main/java/org/apache/gravitino/storage/relational/service/TagMetaService.java:
##########
@@ -625,6 +678,66 @@ private static void validateAllowedValue(TagPO tagPO,
TagValue tagValue)
Arrays.toString(allowedValues));
}
+ void lockMetalakeForTagCreate(MetalakePO observedMetalakePO) {
Review Comment:
**test-coverage**: `lockMetalakeForTagCreate`'s `NoSuchEntityException`
branch (fired when the parent metalake is renamed or removed concurrently with
a tag insert) has no dedicated test.
CLAUDE.md: "Write unit tests for ALL new logic. NO tests = NO merge." None
of the three new tests added in `TestTagMetaService.java`'s diff hunk
(`testTagAlterDeleteAndOverwriteUseMonotonicVersion`,
`testTagAlterReportsOptimisticLockConflict`,
`testStaleTagDeleteRollsBackRelationshipCleanup`) exercise a concurrent
metalake rename/delete racing a tag `insertTag` call, so a regression in this
new locking path would not be caught by CI.
##########
core/src/main/java/org/apache/gravitino/storage/relational/service/TagMetaService.java:
##########
@@ -625,6 +678,66 @@ private static void validateAllowedValue(TagPO tagPO,
TagValue tagValue)
Arrays.toString(allowedValues));
}
+ void lockMetalakeForTagCreate(MetalakePO observedMetalakePO) {
+ OccWriteSupport.lockParentForChildWrite(
+ observedMetalakePO.getMetalakeName(),
+ Entity.EntityType.METALAKE,
+ () ->
+ SessionUtils.getWithoutCommit(
+ MetalakeMetaMapper.class,
+ mapper ->
+
mapper.selectMetalakeMetaByIdForShare(observedMetalakePO.getMetalakeId())),
+ null,
+ current -> Objects.equals(current.getMetalakeName(),
observedMetalakePO.getMetalakeName()));
+ }
+
+ private void deleteTagWithVersion(NameIdentifier identifier, TagPO
observedTagPO) {
+ OccWriteSupport.deleteWithVersion(
+ () ->
+ SessionUtils.getWithoutCommit(
+ TagMetaMapper.class,
+ mapper ->
+ mapper.softDeleteTagMetaByIdAndVersion(
+ observedTagPO.getTagId(),
observedTagPO.getCurrentVersion())),
+ () -> tagWriteFailure(identifier, observedTagPO));
+ }
+
+ private RuntimeException tagWriteFailure(NameIdentifier identifier, TagPO
observedTagPO) {
Review Comment:
**test-coverage**: `tagWriteFailure`'s `NoSuchEntityException` branch (tag
genuinely gone/renamed, as opposed to a pure version conflict) has no dedicated
test — only the `OptimisticLockException` branch is exercised.
CLAUDE.md: "NO tests = NO merge." `tagWriteFailure` returns
`NoSuchEntityException` when the re-read current PO is null or its identity
(tagName/metalakeId) differs from the observed one, or
`OptimisticLockException` otherwise. The new tests
(`testTagAlterReportsOptimisticLockConflict`,
`testStaleTagDeleteRollsBackRelationshipCleanup`) only exercise the
`OptimisticLockException` path; the identity-mismatch/not-found path is
untested, so a bug that mis-classifies a deleted/renamed tag as a plain
conflict (or vice versa) would go undetected.
##########
core/src/main/java/org/apache/gravitino/storage/relational/service/TagMetaService.java:
##########
@@ -156,27 +174,47 @@ public <E extends Entity & HasIdentifier> TagEntity
updateTag(
@Monitored(metricsSource = GRAVITINO_RELATIONAL_STORE_METRIC_NAME,
baseMetricName = "deleteTag")
public boolean deleteTag(NameIdentifier identifier) {
- String metalakeName = identifier.namespace().level(0);
- int[] tagDeletedCount = new int[] {0};
- int[] tagMetadataObjectRelDeletedCount = new int[] {0};
+ TagPO tagPO;
+ try {
+ tagPO = getTagPOByMetalakeAndName(identifier.namespace().level(0),
identifier.name());
+ } catch (NoSuchEntityException e) {
+ return false;
+ }
+ return deleteTag(identifier, tagPO);
+ }
+
+ boolean deleteTag(NameIdentifier identifier, TagPO tagPO) {
Review Comment:
**conventions**: New package-private overload `boolean
deleteTag(NameIdentifier, TagPO)` is spliced between the public
`deleteTag(NameIdentifier)` (line 176) and the public
`listTagsForMetadataObject` (line 223), breaking the visibility grouping
CLAUDE.md requires.
CLAUDE.md's repo-root rules state: "Class Member Ordering: Follow the order:
... 5. Methods (Group by visibility, putting `private` methods at the end)."
This new package-private method sits directly between two public methods, so
the sequence becomes public -> package-private -> public instead of all public
methods being grouped together — the same class of violation found in the
companion Model-OCC PR (#12649).
--
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]
