dimas-b commented on code in PR #5094:
URL: https://github.com/apache/polaris/pull/5094#discussion_r3617905331
##########
persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/JdbcBasePersistenceImpl.java:
##########
@@ -205,10 +203,14 @@ private void persistEntity(
// 1. PRIMARY KEY violated
// 2. UNIQUE CONSTRAINT on (realm_id, catalog_id, parent_id,
type_code, name) violated
// With SERIALIZABLE isolation, the conflicting entity may _not_ be
visible and
- // existingEntity can be null, which would cause an NPE in
- // EntityAlreadyExistsException.message().
- throw new EntityAlreadyExistsException(
- existingEntity != null ? existingEntity : entity, e);
+ // existingEntity can be null. We cannot distinguish a same-id
idempotent retry from a
+ // genuine name collision in that case, so we must report a
concurrency conflict rather
+ // than fabricate the entity we were trying to create.
+ if (existingEntity != null) {
Review Comment:
This is still not 100% correct, I think. The `existingEntity` is loaded in a
different Tx, so there's no guarantee that it is the root cause of the conflict
in current Tx 🤷
That said, I think the tradeoff is that the change code is more correct than
the previous code 😅 I think it's a reasonable 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]
