jerryshao commented on code in PR #12551:
URL: https://github.com/apache/gravitino/pull/12551#discussion_r3853143794
##########
core/src/main/java/org/apache/gravitino/catalog/OperationDispatcher.java:
##########
@@ -214,11 +215,23 @@ protected StringIdentifier
getStringIdFromProperties(Map<String, String> propert
}
}
+ /**
+ * Runs a store operation as a best-effort side effect of the request.
+ *
+ * <p>Every failure is logged and reported as a null result, because the
external catalog is the
+ * source of truth on these paths: a load that imports or repairs the
Gravitino copy must still
+ * return the entity it read, and the next load repairs what this one could
not write.
+ */
protected <R extends HasIdentifier> R operateOnEntity(
NameIdentifier ident, ThrowableFunction<NameIdentifier, R> fn, String
opName, long id) {
R ret = null;
try {
ret = fn.apply(ident);
+ } catch (OptimisticLockException e) {
Review Comment:
**Altitude: this special case depends on an unenforced fact about callers.**
The comment concedes `operateOnEntity` is safe to swallow
`OptimisticLockException` in only because "managed operations do not use this
best-effort helper" — a fact about *callers*, not something this generic
dispatcher-level helper can verify or enforce. If any future managed-table code
path (or a copy/paste in a new dispatcher) is ever routed through
`operateOnEntity`, an OCC conflict on that path would silently be downgraded to
a log warning instead of surfacing — exactly the class of bug this PR is fixing
everywhere else. Consider making the strict-vs-best-effort choice explicit at
each call site (e.g. two named helpers,
`operateOnEntityBestEffort`/`operateOnEntityStrict`, or a boolean parameter)
rather than relying on this dispatcher-wide catch ordering plus a code comment
to keep the invariant true.
##########
core/src/main/java/org/apache/gravitino/storage/relational/mapper/provider/base/TableMetaBaseSQLProvider.java:
##########
@@ -178,6 +178,29 @@ public String selectTableMetaById(@Param("tableId") Long
tableId) {
+ " WHERE tm.table_id = #{tableId} AND tm.deleted_at = 0";
}
+ /**
+ * Returns the active table metadata row and holds it exclusively for the
current transaction.
+ *
+ * <p>Unlike the metalake, catalog and schema providers, this cannot be
written as {@code
+ * selectTableMetaById(id) + " FOR UPDATE"}: that select LEFT JOINs {@code
table_version_info},
+ * and locking the nullable side of an outer join is rejected by PostgreSQL
and locks the wrong
+ * rows on MySQL. The projection is therefore spelled out for {@code
table_meta} alone, and the
+ * returned row carries only the identity and version columns its callers
read.
+ *
+ * @param tableId the table ID
+ * @return the locking select SQL
+ */
+ public String selectTableMetaByIdForUpdate(@Param("tableId") Long tableId) {
Review Comment:
**Reuse (minor): this locking-select pattern is now written a third time.**
`selectTableMetaByIdForUpdate` (single-table "SELECT ... WHERE id = #{id}
AND deleted_at = 0 FOR UPDATE" projection) mirrors
`selectSchemaMetaByIdForUpdate` and `selectCatalogMetaByIdForUpdate` added by
the earlier schema/catalog OCC work — same shape modulo column/table names,
with no shared SQL-fragment helper across the three providers. Not blocking,
just flagging alongside the similar duplication in
`TableMetaService.tableWriteFailure` for a possible follow-up cleanup.
--
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]
