yuqi1129 commented on code in PR #13792:
URL: https://github.com/apache/gravitino/pull/13792#discussion_r4270321208
##########
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:
Fixed in 1821d51f66. Lance updateTableWithCasRetry now checks
ExceptionUtils.isLockWaitTimeout on the preserved cause and immediately
rethrows that same conflict, without backoff or another store.update. Genuine
lost-CAS conflicts keep the existing bounded retry behavior.
testLoadDoesNotRetryLockWaitTimeout covers both repairTableMetadata and
recordCheckedEmptyVersion via loadTable, with the MySQL/H2/PostgreSQL codes:
one update, no backoff, and the original exception/cause. Removing the guard
makes all six cases fail; restored code passes. This removes the fivefold
amplification, while one attempt can still consume the backend lock-wait
timeout.
##########
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:
The timeout-first precedence was not intentional. Changed in 1821d51f66:
checkSQLException now gives a duplicate-key violation precedence across the
cause/next-exception graph, preserving the original failure as cause and the
existing already-exists message.
lockWaitTimeoutAsConflict/throwIfLockWaitTimeout also leave mixed chains for
that classification, so the tag/model association helpers cannot bypass it. The
regression test checks both chain orders for all three backend code sets;
removing the precedence makes it fail. Storage service tests passed on H2,
MySQL and PostgreSQL. I have not reproduced a mixed duplicate-key/timeout chain
through a production batch path, so this is a defensive classification
guarantee rather than a claim about a particular driver producing that chain.
That evidence boundary is also recorded in the PR description.
##########
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:
Fixed both sibling logs and their comments in 1821d51f66. Job cleanup and
status-update deferral now describe a concurrent operation, covering both CAS
losses and lock-wait timeouts without claiming the row changed. The deferral
behavior is unchanged; all 75 TestJobManager cases passed, including the final
restored-code run.
--
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]
