jerryshao commented on PR #13198:
URL: https://github.com/apache/gravitino/pull/13198#issuecomment-5795238245
Verdict: ship it
\nA: observes t as (id=7, v=3), external drop of t succeeds\n t is
re-created out of band, still carrying gravitino.identifier=7\n any load of t
in this window -> importTable -> store.put(id=7, overwrite) -> row 7 at v=4\nA:
store delete -> id 7 == 7 -> passes -> the registration of a table that exists
again is deleted,\n together with its columns, tags, owner and
policies\n```\n\nThe id+version comparison rejected exactly this. The window
needs an out-of-band re-create plus a load between the external drop returning
and the store delete, so I do not think it blocks the PR, but it is the one
case the id alone cannot tell apart from a plain alter. If it is accepted, a
sentence in the `EntityVersion` javadoc would help, since `version()` is now
read by no fence at all and a store implementer could reasonably assume it is
still a precondition.\n\nVerified by: read
`OccWriteSupport.checkExpectedIdentity` and all five meta-service callers at
HEAD 65d47f5; read `importTable` in fu
ll (`TableOperationDispatcher.java:519-575`) and grepped `stringId.id()`
across `core/src/main/java/org/apache/gravitino/catalog` for the other three
import paths; grepped `expected.version()` across `core/src/main` and found no
remaining production reader."
},
{
"path":
"core/src/main/java/org/apache/gravitino/utils/SchemaEntityCleaner.java",
"line": 97,
"side": "RIGHT",
"severity": "Nit",
"body": "[Nit] The outcome this commit was written to produce is
logged as a failure.\n\n`store.delete(..., outermostObserved)` throws
`OptimisticLockException` precisely when a concurrent re-creation is caught,
which is the success case for this fix. That exception falls into the generic
`catch (Exception e)` below and is logged at WARN as `Failed to clean up
orphaned schema entities starting from {}`, with a stack trace. Under HA that
turns a correct, expected result into recurring warning noise that reads like a
bug.\n\nSuggest catching `OptimisticLockException` ahead of the catch-all and
logging it at debug or info, with a message saying the registration was
re-created and was deliberately left alone.\n\nVerified by: read
`SchemaEntityCleaner.java` in full at HEAD 65d47f5 (the delete at line 97, the
`NoSuchEntityException` catch at 98, the catch-all WARN at 100-102) and
confirmed `OptimisticLockException extends GravitinoRuntimeException`, so it
reaches that catch; `Test
SchemaEntityCleaner.testRecreatedSchemaSurvivesCleanup` drives this exact
path."
},
{
"path":
"core/src/test/java/org/apache/gravitino/utils/TestSchemaEntityCleaner.java",
"line": 87,
"side": "RIGHT",
"severity": "Nit",
"body": "[Nit] Both cases here walk a single scope, so the
hierarchical path stays uncovered.\n\n`NameIdentifier.of(\"metalake\",
\"catalog\", \"schema\")` has no schema separator, so
`HierarchicalSchemaUtil.allScopes` returns one candidate and the loop body runs
once. The path that `SchemaOperationDispatcher.java:589` drives is the other
one: `includeSelf = false` with a multi-level schema name, where the loop keeps
only the outermost orphan's observation and discards the inner ones before
breaking on the first candidate that still exists.\n\nTwo cheap additions would
pin the new logic: a nested name whose inner candidate is absent from the
catalog and whose outer candidate is present (asserting the delete targets the
outer one with the outer one's observation), and a case where the outermost
orphan has no store row, which now returns early at
`SchemaEntityCleaner.java:93` instead of reaching the delete.\n\nVerified by:
read `TestSchemaEntityCleaner` in full and `SchemaEnti
tyCleaner.deleteOrphanedSchemaEntities` at HEAD 65d47f5; read
`HierarchicalSchemaUtil.allScopes`, which adds the full name first and then
strips one separator at a time."
}
]
}
```
---
_Generated by [Claude Code](https://claude.ai/code)_
--
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]
