yuqi1129 commented on code in PR #14280:
URL: https://github.com/apache/gravitino/pull/14280#discussion_r4398183823
##########
core/src/main/java/org/apache/gravitino/catalog/TableOperationDispatcher.java:
##########
@@ -1054,8 +1059,13 @@ private TableEntity updateColumnsIfNecessaryWhenLoad(
// stored columns since, e.g. renamed a column while keeping its id. So
the columns are matched
// again against the entity being updated, which is the latest stored one.
This narrows the
// load/alter race but does not close it: if this load read the catalog
before a concurrent
- // alter and the store after it, the catalog snapshot is the stale side.
Closing that needs
- // alterTable and loadTable to exclude each other, which is tracked
separately.
+ // alter and the store after it, the catalog snapshot is the stale side,
and a column that the
+ // alter renamed is stored as dropped plus added, so it loses its id and
its tags.
+ //
+ // A tree lock cannot close this, because tree locks are local to one
server. Closing it needs
+ // the alter to record in the store that it is in progress before it calls
the catalog, and
+ // the load to read the stored version and that mark before it reads the
catalog, then write
+ // only if both are unchanged. That needs a new EntityStore API and is
tracked in #14279.
Review Comment:
Updated in 548b6c057e. The comment now explicitly requires load to skip the
store write while the in-progress mark is set, and to write only if the version
and operation epoch are unchanged and no in-progress mark is set, matching
#14279. This remains a documentation-only change. Core Spotless checks and an
independent re-review passed.
##########
core/src/main/java/org/apache/gravitino/catalog/TableOperationDispatcher.java:
##########
@@ -1054,8 +1059,13 @@ private TableEntity updateColumnsIfNecessaryWhenLoad(
// stored columns since, e.g. renamed a column while keeping its id. So
the columns are matched
// again against the entity being updated, which is the latest stored one.
This narrows the
// load/alter race but does not close it: if this load read the catalog
before a concurrent
- // alter and the store after it, the catalog snapshot is the stale side.
Closing that needs
- // alterTable and loadTable to exclude each other, which is tracked
separately.
+ // alter and the store after it, the catalog snapshot is the stale side,
and a column that the
+ // alter renamed is stored as dropped plus added, so it loses its id and
its tags.
+ //
+ // A tree lock cannot close this, because tree locks are local to one
server. Closing it needs
+ // the alter to record in the store that it is in progress before it calls
the catalog, and
+ // the load to read the stored version and that mark before it reads the
catalog, then write
+ // only if both are unchanged. That needs a new EntityStore API and is
tracked in #14279.
Review Comment:
done.
--
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]
