yuqi1129 commented on code in PR #12781:
URL: https://github.com/apache/gravitino/pull/12781#discussion_r3904183010
##########
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:
Documented rather than restructured. `deleteTag` now carries a comment
stating that the version-checked delete must stay first, because that is what
decides whether the delete wins and a losing delete rolls the transaction back
before any dependent row is touched.
`OccWriteSupport.deleteChildrenWithVersions` does not fit these five calls:
it asserts that the affected row count equals the size of a known child list,
while these are blanket soft deletes for one tag ID where zero rows is a
legitimate outcome (a tag with no assignments, no policies, no owner and no
securable object). The comment says that too, so the next reader does not reach
for the helper by reflex.
##########
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:
Added `testDeleteTagCleansEveryDependentRelation`: it sets up an assignment
on a catalog, a policy-tag relation, a policy applied to the tag itself, an
owner relation and a role whose securable object names the tag, asserts one
active row in each of the five tables, deletes the tag and asserts all five are
gone. A wrong mapper method or a wrong id/type argument in any of the four new
cascade calls now fails 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) {
+ 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:
All three points addressed.
reuse/altitude: `lockTagsForAssignment` and `PolicyTagRelService.lockTag`
both route through a new `TagMetaService.lockTags`, which classifies through
`OccWriteSupport.lockParentForChildWrite`. The three hand-rolled copies are now
one call site of the shared helper.
efficiency: `lockTags` issues a single `listTagPOsByTagIdsForUpdate` (`WHERE
tag_id IN (...) ORDER BY tag_id FOR UPDATE`) and matches the locked rows
against the observed ones, so a metadata object with 10 tags costs one round
trip instead of ten, with the same tag-ID lock order.
test-coverage: the missing-row classification is covered by
`testDeleteOfAlreadyDeletedTagReportsMissingTagNotConflict` and by
`TestOccWriteSupport`. A tag renamed by another transaction in the middle of
this one still is not covered: it needs two connections interleaved inside one
transaction, which this suite has no harness for.
##########
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:
Fixed. The package-private overload now sits below the public methods,
before the private helpers.
##########
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:
Added `testTagCreateIsFencedByParentMetalake`, which asserts both
`insertTag(tag, false)` and `insertTag(tag, true)` fail with
`NoSuchEntityException` when the parent metalake is not there.
The narrower branch where the metalake is renamed or dropped by another
transaction between the lookup and the locking read needs two interleaved
connections, which this suite cannot express; the classification itself is
covered by `TestOccWriteSupport.testLockParentForChildWriteNotFoundThrows` and
`...IdentityMismatchThrows`.
##########
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:
Added `testDeleteOfAlreadyDeletedTagReportsMissingTagNotConflict`: it
captures the observed row, deletes the tag, then replays the delete with the
stale row and asserts `NoSuchEntityException` rather than
`OptimisticLockException`, which is exactly the mis-classification you describe.
--
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]
