jerryshao commented on code in PR #13792:
URL: https://github.com/apache/gravitino/pull/13792#discussion_r4278153411
##########
core/src/main/java/org/apache/gravitino/storage/relational/utils/ExceptionUtils.java:
##########
@@ -20,17 +20,65 @@
import java.io.IOException;
import java.sql.SQLException;
+import java.util.ArrayDeque;
+import java.util.Collections;
+import java.util.Deque;
+import java.util.IdentityHashMap;
import java.util.Locale;
+import java.util.Set;
+import javax.annotation.Nullable;
import org.apache.gravitino.Entity;
+import org.apache.gravitino.EntityAlreadyExistsException;
import org.apache.gravitino.NameIdentifier;
import org.apache.gravitino.exceptions.OptimisticLockException;
import
org.apache.gravitino.storage.relational.converters.SQLExceptionConverterFactory;
public class ExceptionUtils {
+
+ /** MySQL error code of a lock wait timeout (ER_LOCK_WAIT_TIMEOUT). */
+ private static final int MYSQL_LOCK_WAIT_TIMEOUT = 1205;
+
+ /** H2 error code of a lock wait timeout (LOCK_TIMEOUT_1, SQLState HYT00). */
+ private static final int H2_LOCK_TIMEOUT = 50200;
+
+ /** PostgreSQL SQLState of a lock that is not available within lock_timeout.
*/
+ private static final String POSTGRESQL_LOCK_NOT_AVAILABLE = "55P03";
+
+ private static final Set<String> LOCK_WAIT_TIMEOUT_SQL_STATES =
+ Set.of(POSTGRESQL_LOCK_NOT_AVAILABLE);
+
+ private static final Set<Integer> LOCK_WAIT_TIMEOUT_VENDOR_CODES =
+ Set.of(MYSQL_LOCK_WAIT_TIMEOUT, H2_LOCK_TIMEOUT);
+
+ private static final Set<String> DUPLICATE_KEY_SQL_STATES = Set.of("23505");
+ private static final Set<Integer> DUPLICATE_KEY_VENDOR_CODES = Set.of(1062);
+
private ExceptionUtils() {}
+ /**
+ * Converts a failure of a write whose cause is a {@link SQLException} into
the matching Gravitino
+ * exception. A lock wait timeout becomes an {@link
OptimisticLockException}, see {@link
+ * #lockWaitTimeoutAsConflict(RuntimeException, String)}. It is classified
here, before the
+ * backend converter, because the converter reports an unrecognised SQL
error as an {@link
+ * IOException}, which callers turn into an internal error. A duplicate-key
failure takes
+ * precedence over a timeout in the same exception chain: retrying cannot
resolve that violation.
+ *
+ * @param re The failure of the write.
+ * @param type The type of the entity being written.
+ * @param entityName The name of the entity being written.
+ * @throws IOException If the SQL error has no more specific mapping.
+ * @throws EntityAlreadyExistsException If the failure contains a
duplicate-key violation.
+ * @throws OptimisticLockException If the failure contains a lock wait
timeout without a
+ * duplicate-key violation.
+ */
public static void checkSQLException(
RuntimeException re, Entity.EntityType type, String entityName) throws
IOException {
+ if (isDuplicateKey(re)) {
+ throw new EntityAlreadyExistsException(
+ re, "The %s entity: %s already exists.", type.name(), entityName);
Review Comment:
[Nit] The duplicate-key codes now live in three places, and the backend
converters' duplicate-key arms are no longer reachable from here.
This pre-check short-circuits `SQLExceptionConverterFactory` for every
duplicate key, not only for a mixed chain. It is behaviour-preserving today:
`MySQLExceptionConverter.java:43`, `H2ExceptionConverter.java:42-44` and
`PostgreSQLExceptionConverter.java:43` all build the same
`EntityAlreadyExistsException` with the same message, and the only observable
change is that the cause is now the wrapping `RuntimeException` instead of the
`SQLException` (`TestExceptionUtils.java:181` pins that).
What is left behind is three independent definitions of the same codes:
`ExceptionUtils.java:53-54`, `StatisticMetaService.isDuplicateKey`
(`StatisticMetaService.java:362`, which also rebuilds `Set.of(...)` on each
call -- the allocation hoisted out of the timeout sets earlier in this PR), and
the converters. A converter that later recognises another duplicate-key code
would be silently bypassed here. There is also a small asymmetry:
`H2ExceptionConverter` matches vendor code 23505 while this matches SQLState
23505 -- equivalent for the H2 driver, which sets both, but the two will not
stay in step by construction.
Suggestion: make `isDuplicateKey` public and have `StatisticMetaService`
call it, so the duplicate-key codes sit in one place next to the lock-timeout
ones.
Verified by: read `ExceptionUtils.java:45-84` and `:238-244`,
`StatisticMetaService.java:284-300` and `:358-364`, and all three converters on
this head.
##########
core/src/main/java/org/apache/gravitino/storage/relational/utils/ExceptionUtils.java:
##########
@@ -69,4 +117,129 @@ public static OptimisticLockException
concurrentChildModification(
parentType.name().toLowerCase(Locale.ROOT),
parentIdentifier);
}
+
+ /**
+ * Throws a write that failed on a lock wait timeout as a conflict, and
returns otherwise. A
+ * service calls this before {@link #checkSQLException} when the type and
name it reports for
+ * other SQL errors do not describe what was written, for example the
metadata object of a tag
+ * association.
+ *
+ * @param re The failure of the write.
+ * @param target What was being written, as the subject of the message.
+ * @throws OptimisticLockException If the failure is a lock wait timeout
without a duplicate-key
+ * violation.
+ * @see #lockWaitTimeoutAsConflict(RuntimeException, String)
+ */
+ public static void throwIfLockWaitTimeout(RuntimeException re, String
target) {
+ RuntimeException conflict = lockWaitTimeoutAsConflict(re, target);
+ if (conflict != re) {
+ throw conflict;
+ }
+ }
+
+ /**
+ * Returns whether a failure was caused by a statement that waited on a row
lock longer than the
+ * backend's lock wait timeout. MySQL reports error code 1205, H2 error code
50200 (SQLState
+ * HYT00), and PostgreSQL SQLState 55P03 when lock_timeout is set. The MySQL
and H2 vendor codes
+ * are matched rather than their SQLState, because the MySQL driver reports
a lock wait timeout as
+ * SQLState 40001 or HY000 depending on its version, and 40001 is also used
for deadlocks.
+ *
+ * <p>Deadlocks are deliberately not included: a deadlock on a write path
points at an
+ * inconsistent lock order, which should be fixed on that path rather than
reported as a conflict.
+ *
+ * @param failure The failure to inspect, including its causes and chained
SQL exceptions, or
+ * null.
+ * @return {@code true} if a lock wait timeout is found.
+ */
+ public static boolean isLockWaitTimeout(@Nullable Throwable failure) {
+ return hasSqlException(failure, LOCK_WAIT_TIMEOUT_SQL_STATES,
LOCK_WAIT_TIMEOUT_VENDOR_CODES);
+ }
+
+ /**
+ * Returns whether a failure contains a {@link SQLException} with one of the
given SQLStates or
+ * vendor error codes. Both causes and {@link
SQLException#getNextException()} are inspected;
+ * repeated exceptions are visited only once, even when the chains contain
cycles.
+ *
+ * @param failure The failure to inspect, including its causes and chained
SQL exceptions, or
+ * null.
+ * @param sqlStates The SQLStates to match.
+ * @param vendorErrorCodes The vendor error codes to match.
+ * @return {@code true} if a matching {@link SQLException} is found.
+ */
+ public static boolean hasSqlException(
+ @Nullable Throwable failure, Set<String> sqlStates, Set<Integer>
vendorErrorCodes) {
+ if (failure == null) {
+ return false;
+ }
+ Set<Throwable> visited = Collections.newSetFromMap(new
IdentityHashMap<>());
+ Deque<Throwable> pending = new ArrayDeque<>();
+ pending.add(failure);
+ while (!pending.isEmpty()) {
+ Throwable cause = pending.removeLast();
+ if (!visited.add(cause)) {
+ continue;
+ }
+ if (cause instanceof SQLException) {
+ SQLException sql = (SQLException) cause;
+ if ((sql.getSQLState() != null &&
sqlStates.contains(sql.getSQLState()))
+ || vendorErrorCodes.contains(sql.getErrorCode())) {
+ return true;
+ }
+ if (sql.getNextException() != null) {
+ pending.add(sql.getNextException());
+ }
+ }
+ if (cause.getCause() != null) {
+ pending.add(cause.getCause());
+ }
+ }
+ return false;
+ }
+
+ /**
+ * Reports a write of one entity that failed on a lock wait timeout as a
conflict.
+ *
+ * @param failure The failure of the write.
+ * @param type The type of the entity being written.
+ * @param identifier The identifier of the entity being written.
+ * @return An {@link OptimisticLockException} for a lock wait timeout,
otherwise {@code failure}.
+ * @see #lockWaitTimeoutAsConflict(RuntimeException, String)
+ */
+ public static RuntimeException lockWaitTimeoutAsConflict(
+ RuntimeException failure, Entity.EntityType type, NameIdentifier
identifier) {
+ return lockWaitTimeoutAsConflict(
+ failure, String.format("The %s %s",
type.name().toLowerCase(Locale.ROOT), identifier));
+ }
+
+ /**
+ * Reports a write that failed on a lock wait timeout as a conflict. The
statement waited behind a
+ * concurrent writer of the same rows until the backend aborted it, and the
store rolls the
+ * transaction back, so the store write can be retried. Callers must account
for any external side
+ * effects that occurred before the store write before retrying the whole
operation. Any other
+ * failure, including one that is already an {@link
OptimisticLockException}, is returned
+ * unchanged. A failure containing a duplicate-key violation is also
returned unchanged so that
+ * the service can report the non-retryable error instead of a timeout
elsewhere in the chain.
+ *
+ * @param failure The failure of the write.
+ * @param target What was being written, as the subject of the message, for
example "The table
+ * m1.c1.s1.t1".
+ * @return An {@link OptimisticLockException} for a lock wait timeout,
otherwise {@code failure}.
+ */
+ public static RuntimeException lockWaitTimeoutAsConflict(
+ RuntimeException failure, String target) {
+ if (failure instanceof OptimisticLockException
+ || !isLockWaitTimeout(failure)
+ || isDuplicateKey(failure)) {
Review Comment:
[Question] The duplicate-key precedence stops at the `checkSQLException`
path; on the store write paths a mixed chain now gets neither classification.
This guard returns the failure unchanged when the chain also carries a
duplicate key, but unlike `checkSQLException` this method has no duplicate-key
branch of its own. It is the only classifier on the `RelationalEntityStore`
write paths -- put, update, delete, deleteAndGet, batchPut, batchDelete and the
relation writes (`RelationalEntityStore.java:232, 252, 332, 348, 454, 488, 530,
565, 586, 601`) -- which the PR description lists as exactly the paths that
deliberately do not go through `checkSQLException`. On those, a mixed chain now
escapes as the raw backend `RuntimeException` (a MyBatis
`PersistenceException`), i.e. HTTP 500, rather than the non-retryable
already-exists error the javadoc two lines above promises the service will
report.
That is the pre-PR outcome, so nothing regresses; the issue is that the
javadoc describes a reporting step that only exists for `checkSQLException`
callers. Either map the duplicate key here too (the same
`EntityAlreadyExistsException` `checkSQLException` builds), or narrow the
javadoc to say the mixed chain is deferred to `checkSQLException` callers and
stays unclassified elsewhere. Given the chain has not been reproduced through a
production batch path, documenting it is probably enough.
Verified by: read `ExceptionUtils.java:74-84` and `:217-245` and every
`lockWaitTimeoutAsConflict` call site in `RelationalEntityStore.java` on this
head; grep over `core/src/main` shows `checkSQLException` is the only caller
that classifies a duplicate key.
--
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]
