jerryshao commented on code in PR #13792:
URL: https://github.com/apache/gravitino/pull/13792#discussion_r4270094545
##########
core/src/main/java/org/apache/gravitino/storage/relational/RelationalEntityStore.java:
##########
@@ -230,7 +245,12 @@ public <E extends Entity & HasIdentifier> void put(E e,
boolean overwritten)
public <E extends Entity & HasIdentifier> E update(
NameIdentifier ident, Class<E> type, Entity.EntityType entityType,
Function<E, E> updater)
throws IOException, NoSuchEntityException, EntityAlreadyExistsException {
- E updatedEntity = backend.update(ident, entityType, updater);
+ E updatedEntity;
+ try {
+ updatedEntity = backend.update(ident, entityType, updater);
+ } catch (RuntimeException re) {
+ throw ExceptionUtils.lockWaitTimeoutAsConflict(re, entityType, ident);
Review Comment:
[Important] This conversion makes a lock-wait timeout indistinguishable from
a version CAS loss, and one caller retries on that distinction.
LanceTableOperations.updateTableWithCasRetry
(catalogs/catalog-lakehouse-generic/.../lance/LanceTableOperations.java:649-677)
treats every OptimisticLockException from store.update as "another repair won
the same race", and retries up to REPAIR_UPDATE_MAX_ATTEMPTS = 5 (:93) with a
10-100 ms backoff (:96-99). Its callers are on the load path:
repairTableMetadata (:620) and recordCheckedEmptyVersion (:698), both reached
when the stored dataset version differs from the dataset.
Before this PR a lock-wait timeout there failed once with a 500. Now every
one of the 5 attempts can block for the full backend timeout before being
retried: about 1-2 s each on H2, but 50 s each on MySQL with the default
innodb_lock_wait_timeout, so a single loadTable can hold its request thread for
roughly 4 minutes and then still return 409. A retry also cannot help, since
the row is held by a writer, not by a competing repairer.
Suggestion: keep the timeout recognisable at the call site and do not retry
it, for example by checking ExceptionUtils.isLockWaitTimeout(e) inside the loop
(the cause is preserved, so this works), or by giving the timeout its own
exception type; alternatively bound the loop by total wall time instead of
attempt count. A test that throws a lock-wait timeout into
updateTableWithCasRetry would pin the behaviour.
Verified by: read RelationalEntityStore.java:245-256, then the loop, its
constants and both callers in LanceTableOperations.java on this branch; grep
showed this is the only retry-on-OptimisticLockException loop in main code.
##########
core/src/main/java/org/apache/gravitino/storage/relational/utils/ExceptionUtils.java:
##########
@@ -20,17 +20,54 @@
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.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 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.
+ *
+ * @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 OptimisticLockException If the failure contains a lock wait
timeout.
+ */
public static void checkSQLException(
RuntimeException re, Entity.EntityType type, String entityName) throws
IOException {
+ throwIfLockWaitTimeout(
+ re, String.format("The %s %s", type.name().toLowerCase(Locale.ROOT),
entityName));
Review Comment:
[Question] Classifying the timeout before the backend converter also means
it wins over a unique-key violation in the same failure.
isLockWaitTimeout walks the whole cause graph plus every getNextException
(:157-185), so for a batched write whose chained SQLExceptions carry both a
duplicate key and a lock-wait timeout, this call throws a retryable 409 and the
already-exists error is never produced. The batch paths that reach
checkSQLException are the ones this PR touches: ModelVersionMetaService
alias-rel inserts and the tag-association writes.
StatisticMetaService.translateConflict deliberately uses the opposite order,
checking isDuplicateKey (:289) before isConcurrencyFailure (:292), so a request
that can never succeed is not advertised as retryable.
Is the order here intentional, and can one driver report both in a single
chain? If it can, checking the duplicate key first would match the statistics
path and avoid telling the caller to retry something that will always fail.
Verified by: read ExceptionUtils.java:66-72 and 157-185, and
StatisticMetaService.java:284-300 on this branch.
##########
core/src/main/java/org/apache/gravitino/tag/TagManager.java:
##########
@@ -179,7 +179,7 @@ public Tag alterTag(String metalake, String name,
TagChange... changes)
// and the REST layer maps it to a conflict. Log it here so the
operator-facing record
// still names the tag and the metalake.
LOG.warn(
- "Failed to alter tag {} under metalake {} because it changed
concurrently",
+ "Failed to alter tag {} under metalake {} because of a concurrent
operation",
Review Comment:
[Nit] This reword is right, and two sibling logs were missed.
A lock-wait timeout means a concurrent writer held the row, not that the
entity changed, which is why "changed concurrently" became "because of a
concurrent operation" here and at :199. JobManager still logs "changed
concurrently" at :824 (cleanup deferral) and :1347 (status-update deferral),
and both catch OptimisticLockException around entityStore.delete/update, so
after this PR they are reachable for a lock-wait timeout too. Same wording
there would keep the operator-facing record accurate.
Verified by: read TagManager.java:175-202 and JobManager.java:818-830 and
1340-1352 on this branch.
--
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]
