Copilot commented on code in PR #8145:
URL: https://github.com/apache/incubator-seata/pull/8145#discussion_r3440418126
##########
server/src/main/java/org/apache/seata/server/storage/db/lock/LockStoreDataBaseDAO.java:
##########
@@ -390,7 +390,18 @@ protected boolean doAcquireLocks(Connection conn,
List<LockDO> lockDOs) throws S
ps.setInt(8, lockDO.getStatus());
ps.addBatch();
}
- return ps.executeBatch().length == lockDOs.size();
+ // Do not rely on executeBatch().length == size: per the JDBC spec
the length is
+ // not guaranteed to equal the number of statements, and some
drivers (e.g. Dameng/DM)
+ // aggregate the per-statement results, returning an array of a
different length.
+ // Detect failure via EXECUTE_FAILED instead; real conflicts
(duplicate row_key) still
+ // throw SQLIntegrityConstraintViolationException and are handled
by the catch block below.
+ int[] result = ps.executeBatch();
+ for (int updated : result) {
+ if (updated == java.sql.Statement.EXECUTE_FAILED) {
+ return false;
+ }
+ }
Review Comment:
`executeBatch()` can legally return update counts of `0` for statements that
affected no rows. Since this batch is inserting lock rows, treating `0` as
success could allow the method to return `true` even if some lock rows were not
inserted (i.e., the lock was not actually acquired). Align this with
`doAcquireLock()` which checks `executeUpdate() > 0` by also treating `0` as a
failure result (in addition to `EXECUTE_FAILED`).
##########
server/src/main/java/org/apache/seata/server/storage/db/lock/LockStoreDataBaseDAO.java:
##########
@@ -390,7 +390,18 @@ protected boolean doAcquireLocks(Connection conn,
List<LockDO> lockDOs) throws S
ps.setInt(8, lockDO.getStatus());
ps.addBatch();
}
- return ps.executeBatch().length == lockDOs.size();
+ // Do not rely on executeBatch().length == size: per the JDBC spec
the length is
+ // not guaranteed to equal the number of statements, and some
drivers (e.g. Dameng/DM)
+ // aggregate the per-statement results, returning an array of a
different length.
+ // Detect failure via EXECUTE_FAILED instead; real conflicts
(duplicate row_key) still
+ // throw SQLIntegrityConstraintViolationException and are handled
by the catch block below.
+ int[] result = ps.executeBatch();
Review Comment:
This change fixes a driver-compatibility bug, but existing lock-store tests
use H2 and won't cover the aggregated/short `executeBatch()` result scenario
(e.g., DM). Consider adding a unit test (e.g., with a mocked
`PreparedStatement`/`Connection` or a small test subclass) that verifies
`doAcquireLocks()` returns `true` when `executeBatch()` returns a shorter array
with no `EXECUTE_FAILED` values.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
