Copilot commented on code in PR #12781:
URL: https://github.com/apache/gravitino/pull/12781#discussion_r3902554857
##########
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()));
Review Comment:
The predicate dereferences `current.getMetalakeName()` but
`selectMetalakeMetaByIdForShare(...)` can return null (e.g., metalake
concurrently deleted/soft-deleted), which would throw a NullPointerException
during the lock/OCC check. Make the predicate null-safe (treat a null `current`
as a mismatch) so the OCC path can fail deterministically rather than crashing.
##########
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) {
+ List<TagPO> sortedTagPOs = new ArrayList<>(observedTagPOs);
+ sortedTagPOs.sort(Comparator.comparingLong(TagPO::getTagId));
+ List<TagPO> lockedTagPOs = new ArrayList<>(sortedTagPOs.size());
+ for (TagPO observedTagPO : sortedTagPOs) {
+ TagPO lockedTagPO =
+ SessionUtils.getWithoutCommit(
+ TagMetaMapper.class,
+ mapper ->
mapper.selectTagByTagIdForUpdate(observedTagPO.getTagId()));
+ if (lockedTagPO == null
+ || !Objects.equals(lockedTagPO.getTagName(),
observedTagPO.getTagName())
+ || !Objects.equals(lockedTagPO.getMetalakeId(),
observedTagPO.getMetalakeId())) {
+ throw new NoSuchEntityException(
+ NoSuchEntityException.NO_SUCH_ENTITY_MESSAGE,
+ Entity.EntityType.TAG.name().toLowerCase(),
+ observedTagPO.getTagName());
+ }
+ lockedTagPOs.add(lockedTagPO);
+ }
+ return lockedTagPOs;
+ }
Review Comment:
This introduces an N+1 query pattern (one `SELECT ... FOR UPDATE` per tag).
If tag assignments can include many tags, this may become a noticeable
bottleneck. Consider locking in a single query (e.g., `SELECT ... FOR UPDATE
WHERE tag_id IN (...) ORDER BY tag_id`) and then validating the returned set
against `observedTagPOs` to retain the deadlock-avoidance ordering while
reducing round trips.
##########
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())
Review Comment:
The `current -> ...` predicate assumes `current` is non-null, but
`selectTagByTagIdForUpdate(...)` may return null when the tag was concurrently
deleted, which would cause a NullPointerException while building the OCC
failure. Make the predicate null-safe so callers see the intended
optimistic-lock / not-found failure instead of an NPE.
##########
core/src/test/java/org/apache/gravitino/storage/relational/service/TestTagMetaService.java:
##########
@@ -321,6 +327,124 @@ public void testUpdateTag() throws IOException {
Assertions.assertEquals(tagEntity2, loadedTagEntity1);
}
+ @TestTemplate
+ public void testTagAlterDeleteAndOverwriteUseMonotonicVersion() throws
IOException {
+ createAndInsertMakeLake(METALAKE_NAME);
+ TagMetaService tagMetaService = TagMetaService.getInstance();
Review Comment:
The PR description’s 'How was this patch tested?' section is empty, but this
PR adds/updates significant OCC behavior and includes new tests. Please update
the PR description to explicitly list the tests executed (e.g., the relevant
unit/integration test suites) so reviewers and release tooling have a clear
verification record.
--
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]
