jerryshao commented on PR #13198:
URL: https://github.com/apache/gravitino/pull/13198#issuecomment-5794374220
Verdict: blocking issues
\nA: dropTable(s.t) -> cleanup probes s -> s does not exist in the
catalog\nB: creates s externally and registers it, with tables under it\nA:
store.delete(s, SCHEMA, cascade=true) -> B's schema row and every child row are
gone\n```\n\n`SchemaOperationDispatcher.dropSchema` has the same call at line
589, there with `includeSelf = false`, so it can still cascade-delete a
re-created ancestor schema.\n\nThis is pre-existing rather than a regression,
but it is on the very paths this PR fences and the machinery to fix it is now
in place: observe the candidate with `observeRegistration(candidate, SCHEMA)`
before the `schemaExists` probe and hand it to `deleteObservedRegistration`.
Note the cleanup is deliberately best-effort, so an `OptimisticLockException`
there should be swallowed and logged like the other failures, not propagated.
If you would rather keep it out of this PR, it is worth a follow-up issue,
because the fixed paths now advertise a guarantee the cleanup can still break.\n
\nVerified by: read `SchemaEntityCleaner.java` in full at HEAD d99ba95 (the
probe at 75, the delete at 85, the catch-all at 88),
`OrphanedSchemaCleanup.java:44-57`, and grepped every
`deleteOrphanedSchemaEntities` call site in `core/src/main` -
`SchemaOperationDispatcher.java:589`, `TableOperationDispatcher.java:436` and
`:485`, `ViewOperationDispatcher.java:352`."
},
{
"path":
"core/src/main/java/org/apache/gravitino/storage/relational/service/OccWriteSupport.java",
"line": 93,
"start_line": 87,
"side": "RIGHT",
"severity": "Question",
"body": "[Question] Why does the fence compare the version as well as
the id?\n\nEntity ids are generated per row, so a re-creation under the same
name always produces a different id - that alone closes the ABA this PR
targets. Requiring `actualVersion == expected.version()` additionally makes a
drop fail when the *same* entity was merely altered during the external
call:\n\n```\nobserve t -> (id=7, v=3)\nexternal drop of t succeeds\nconcurrent
alterTable(t) bumps the store row to (7, v=4)\nstore delete: (7,4) != (7,3) ->
OptimisticLockException -> 409\n```\n\nThe caller gets an error for a drop that
did succeed externally, and the registration is kept for an object that no
longer exists - the stale-registration state the description says the fence
avoids. Dropping the version from the comparison would not weaken the ABA
guarantee, because the delete that follows is already a CAS on the version read
inside the same call (`TableMetaService.deleteTableWithVersion`, `TableMetaS
ervice.java:362-370`), which still protects the read-to-write window.\n\nThis
bites managed topics hardest: `TopicOperationDispatcher.dropTopic` observes
unconditionally (line 241) and returns `droppedFromStore`, so a concurrent
`alterTopic` turns a successful Kafka delete into a 409 with the row left
behind.\n\nIf failing the drop on a concurrent alter is the intended contract,
it would help to say so in the `EntityVersion` javadoc, since the surrounding
comments only motivate the re-creation case.\n\nVerified by: read
`OccWriteSupport.checkExpectedVersion` at HEAD d99ba95,
`TableMetaService.deleteTable` and `deleteTableWithVersion` (lines 335-370),
`TopicOperationDispatcher.dropTopic` (lines 236-262), and confirmed
`EntityVersion` carries only `(id, version)` with no notion of \"same entity,
newer version\"."
},
{
"path":
"core/src/main/java/org/apache/gravitino/storage/relational/JDBCBackend.java",
"line": 391,
"start_line": 390,
"side": "RIGHT",
"severity": "Nit",
"body": "[Nit] The FUNCTION arms of `getVersion` and the
version-checked `delete` are unreachable in this PR.\n\nNo dispatcher calls
`observeRegistration(..., FUNCTION)` - the only call sites are
`SchemaOperationDispatcher.java:566`, `TopicOperationDispatcher.java:241`,
`ViewOperationDispatcher.java:332` and `TableOperationDispatcher.java:416` and
`:464` - and `ManagedFunctionOperations.java:149` still uses the unconditional
`store.delete(ident, FUNCTION)`. `FunctionMetaService.getFunctionVersion` and
`deleteFunction(ident, expected)` are therefore reached only from
tests.\n\nThat is fine if functions are queued for a follow-up, but it is worth
a sentence in the PR description or a TODO, otherwise the next reader will
assume function drops are fenced when they are not.\n\nVerified by: grepped
`observeRegistration|getFunctionVersion|deleteFunction\\(` across
`core/src/main` and `catalogs/*/src/main` at HEAD d99ba95; the only production
caller of `deleteFunction(ident, expecte
d)` is `JDBCBackend.java:416`, itself reached only from the four-argument
`EntityStore.delete`."
}
]
}
```
---
_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]
