yuqi1129 commented on code in PR #14350:
URL: https://github.com/apache/gravitino/pull/14350#discussion_r4423268595
##########
core/src/main/java/org/apache/gravitino/storage/relational/service/DomainMetaService.java:
##########
@@ -216,32 +219,37 @@ public <E extends Entity & HasIdentifier> DomainEntity
updateDomain(
// name of its backing tag.
DomainPO lockedDomainPO =
mapper.selectDomainMetaById(observedDomainPO.domainId());
DomainEntity oldDomainEntity =
- DomainPO.fromDomainPO(lockedDomainPO, identifier.namespace());
+ fromDomainPOWithParent(lockedDomainPO, identifier.namespace());
DomainEntity updatedDomainEntity = (DomainEntity)
updater.apply((E) oldDomainEntity);
Preconditions.checkArgument(
Objects.equals(oldDomainEntity.id(), updatedDomainEntity.id()),
"The updated domain entity id: %s must have the same id as the
old entity id %s",
updatedDomainEntity.id(),
oldDomainEntity.id());
+ Preconditions.checkArgument(
+ Objects.equals(oldDomainEntity.parent(),
updatedDomainEntity.parent()),
+ "The parent of domain %s cannot be changed by an update, use
DomainRelService",
+ identifier);
DomainPO newDomainPO = DomainPO.updateDomainPO(lockedDomainPO,
updatedDomainEntity);
updateDomainWithVersion(identifier, mapper, lockedDomainPO,
newDomainPO);
storedDomainPO[0] =
mapper.selectDomainMetaById(lockedDomainPO.domainId());
});
- return DomainPO.fromDomainPO(storedDomainPO[0], identifier.namespace());
+ return fromDomainPOWithParent(storedDomainPO[0], identifier.namespace());
Review Comment:
[Important] Materialize the complete update response before committing.
`doWithCommit` has returned by this point, but `fromDomainPOWithParent` now
performs another database query. If that parent lookup fails, `updateDomain`
throws even though its metadata update and version increment have already
committed. The pre-PR conversion only decoded the captured PO; the new helper
adds database I/O after commit. A concurrent move can also make this return
combine the captured row with a later parent.
A temporary probe on H2, MySQL and PostgreSQL injected a failure into the
parent lookup after the update. Before throwing, a separate SQL session could
already read the new comment. The API call failed, while the stored comment was
changed and the version was 2. Normal exception conversion cannot roll that
commit back.
Please construct/capture the complete `DomainEntity`, including its parent,
inside the transaction while the child remains locked, then return that object
after commit without another database lookup. Add a failure regression
demonstrating rollback when response hydration fails before commit.
Validation used actual database commit visibility and a controlled
mapper-query failure, not a real network outage or an external client's retry.
##########
core/src/main/java/org/apache/gravitino/storage/relational/service/DomainMetaService.java:
##########
@@ -482,4 +506,18 @@ private DomainPO getDomainPOByMetalakeAndName(String
metalakeName, String domain
}
return domainPO;
}
+
+ private DomainEntity fromDomainPOWithParent(DomainPO domainPO, Namespace
namespace) {
+ return DomainPO.fromDomainPO(
+ domainPO,
+ namespace,
+
DomainRelService.getInstance().getParentDomainName(domainPO.domainId()).orElse(null));
+ }
+
+ private List<DomainEntity> fromDomainPOsWithParents(
+ List<DomainPO> domainPOs, Namespace namespace) {
+ List<Long> domainIds =
domainPOs.stream().map(DomainPO::domainId).collect(Collectors.toList());
+ return DomainPO.fromDomainPOs(
+ domainPOs, namespace,
DomainRelService.getInstance().batchGetParentDomainNames(domainIds));
Review Comment:
[Important] Read the domain and its parent from one consistent database
snapshot.
The point/list/batch paths fetch domain rows first and resolve their parents
in a separate statement, normally in a different SQL session. An atomic
concurrent transaction can change a child's metadata and move it between these
reads, producing a combination that was never committed. Simply wrapping two
statements in a READ_COMMITTED transaction would not fix this.
On H2, MySQL and PostgreSQL I paused after the root query and atomically
changed `comment=old, parent=OldParent` to `comment=new, parent=NewParent` in
an independent transaction. `backend.get`, service list and service batch each
returned `comment=old, parent=NewParent`.
The symmetric case exists in `DomainRelService.listSubDomains` (lines
125–131): it reads the parent's old name first, then queries children and
annotates them with that name. A probe atomically renamed the parent and
changed the child metadata between these steps; the result contained new child
data with the old parent name.
Please join the live relation/parent into the same SELECT as the domain row
for get/list/batch/sub-domain listing, and project the parent name from that
statement (or provide a genuinely consistent snapshot strategy). Keep lock-only
reads join-free. Add controlled interleaving regressions for these four read
paths.
This is a confirmed storage snapshot problem; I am not claiming an existing
authorization bypass because the Domain manager/authorization consumers are not
wired on this head.
##########
core/src/main/java/org/apache/gravitino/storage/relational/service/DomainMetaService.java:
##########
@@ -311,15 +332,18 @@ boolean deleteDomain(NameIdentifier identifier, DomainPO
domainPO) {
// The metalake lock comes first. Dropping the metalake soft-deletes the
privileges before the
// domains, while this delete changes the domain row before its
privileges; without the lock
- // the two could deadlock on a domain with privileges. The domain row is
locked next, and a row
- // that is gone or renamed makes the delete report the domain as missing
before any dependent
- // row is touched. The version-checked delete keeps the version as a
backstop. The cleanups that
- // follow are blanket soft deletes for this domain ID, so their row counts
carry no information
- // and are not checked.
+ // the two could deadlock on a domain with privileges. The domain row is
locked next, which also
+ // keeps a sub-domain from being attached while the sub-domain check runs.
A row that is gone or
+ // renamed makes the delete report the domain as missing before any
dependent row is touched.
+ // The version-checked delete keeps the version as a backstop. The
cleanups that follow are
+ // blanket soft deletes for this domain ID, so their row counts carry no
information and are not
+ // checked.
try {
SessionUtils.doMultipleWithCommit(
() -> lockMetalakeForDomainWrite(identifier.namespace().level(0),
domainPO.metalakeId()),
() -> deleteDomainWithVersion(identifier,
lockAndGetDomainPO(identifier, domainPO)),
+ () ->
+
DomainRelService.getInstance().deleteDomainRelsOfDeletedDomain(identifier,
domainId),
Review Comment:
[Important] Check for sub-domains before soft-deleting the domain row.
The preceding operation already soft-deletes the domain before this method
counts its children and throws `NonEmptyEntityException`. A standalone call
rolls back, but nested transactions behave differently: `SessionUtils` supports
nesting, and `SqlSessions.handleSessionClose` only performs the JDBC rollback
when the outermost session count reaches zero.
I reproduced this on H2, MySQL and PostgreSQL with an outer
`doMultipleWithCommit` that catches the expected `NonEmptyEntityException` and
then updates an unrelated child comment. The outer commit persists the refused
parent deletion: the parent is missing, its child's active edge remains, and
the parent resolver now returns empty.
Please take the metalake/domain locks, run a read-only nonempty check before
the first mutation, and then perform the domain/edge/owner/privilege cleanup in
the transaction. Split validation from edge cleanup rather than moving a
mutating cleanup ahead of the domain write. Add the nested-and-caught refusal
case as a regression. This can be fixed locally without redesigning transaction
management.
Validation: real service calls and database transactions on all three
backends. This is a storage-contract failure; no existing Domain REST request
is claimed to exercise it, since the manager is still future work.
--
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]
