yuqi1129 commented on PR #13321:
URL: https://github.com/apache/gravitino/pull/13321#issuecomment-5728248063
Checked the port against `main`: the cleanup set matches item for item
(`main` additionally clears `PolicyTagRel`, which does not exist on
`branch-1.3`), `SessionUtils.doMultipleWithCommit` rolls back on the thrown
`NoSuchEntityException`, the reused by-id mappers all exist on 1.3, and the new
SQL hits `idx_tid` / `idx_pid`. The tests count the relation rows directly and
run on H2/MySQL/PG. LGTM overall; two suggestions.
**1. Delete the main row by `id + name` to close the delete-and-recreate
race as well** (`TagMetaService.java` ~L158-176, `PolicyMetaService.java`
~L203-222)
The current sequence is: read the id by name → delete the main row by name →
clean relations by id. The "renamed after the id was read" case is caught by
the 0-row rollback, but "deleted and recreated under the same name" is not: the
by-name delete removes the *new* entity's row while the by-id cleanup removes
the *old* entity's relations, which leaves exactly the orphans this PR is
fixing (as the description notes).
Changing the main-row delete to `WHERE tag_id = #{tagId} AND tag_name =
#{tagName} AND deleted_at = 0` (same for policy) guarantees the row deleted is
the row that was read: a rename fails on the name, a recreate fails on the id,
both roll back and return `false`. It is one more mapper method in the same
shape as the `softDelete...ByTagId` / `...ByPolicyId` ones added here, and it
gives 1.3 the same "delete what you read" semantics `main` gets from the OCC
version check, so the residual-race paragraph in the description can go.
**2. Log the rollback branch**
Reaching `deleted == 0` means a concurrent rename/delete happened; today it
silently returns `false`, indistinguishable from "did not exist". A
`LOG.warn`/`debug` there ("Tag {} changed between id lookup and delete; rolling
back") would help when diagnosing this on a cluster.
Minor, no action needed:
`softDeleteTagMetadataObjectRelsByMetalakeAndTagName` and
`softDeletePolicyMetadataObjectRelsByMetalakeAndPolicyName` no longer have
production callers on 1.3 after this change.
--
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]
