jerryshao commented on code in PR #12824:
URL: https://github.com/apache/gravitino/pull/12824#discussion_r3947688236
##########
core/src/main/java/org/apache/gravitino/storage/relational/service/TagMetaService.java:
##########
@@ -759,25 +759,18 @@ private void insertNewTagWithoutCommit(TagPO tagPO) {
}
private TagPO findAndLockTagForOverwrite(TagPO initializedTagPO) {
Review Comment:
This PR fixes a concurrent-overwrite-of-a-nonexistent-row race in
`ViewMetaService.insertView` - a `SELECT ... FOR UPDATE` matching zero rows
takes no lock under Gravitino's global `READ_COMMITTED` isolation, so two
concurrent overwrite calls for a not-yet-existing row can both fall through to
a plain `INSERT`, and the loser gets `EntityAlreadyExistsException` instead of
a retryable conflict - by catching that exception in `insertView` and
translating it to `OptimisticLockException` when `overwrite=true`.
This refactor makes `findAndLockTagForOverwrite` use the identical
locking-lookup pattern via the new shared
`OccWriteSupport.findAndLockForOverwrite`, so Tag now has the exact same race.
But `TagMetaService.insertTag`'s catch block (unchanged, further up in this
same file) is left on the old, simple `catch (RuntimeException e) {
ExceptionUtils.checkSQLException(...); throw e; }` pattern with no equivalent
translation - two concurrent `insertTag(t, overwritten=true)` calls for a tag
that doesn't exist yet will surface a raw `EntityAlreadyExistsException`
instead of the retryable conflict `insertView` now produces for the identical
scenario.
Same commit that worked out the correct fix for View left Tag's `insertTag`
on the old behavior - worth backporting the same catch-block translation there.
--
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]
