yuqi1129 commented on code in PR #12782:
URL: https://github.com/apache/gravitino/pull/12782#discussion_r3950171133
##########
core/src/main/java/org/apache/gravitino/storage/relational/service/PolicyMetaService.java:
##########
@@ -440,6 +475,217 @@ public int deletePolicyVersionsByRetentionCount(Long
versionRetentionCount, int
return totalDeletedCount;
}
+ /**
+ * Holds the parent metalake row for the rest of the transaction, so a
policy cannot be created
+ * under a metalake that is going away.
+ *
+ * <p>The lock is shared, not exclusive: many policies can be created under
the same metalake at
+ * the same time, while dropping the metalake takes an exclusive lock on
this row, so a drop and a
+ * create cannot overlap.
+ *
+ * <p>The name is compared again because the ID alone cannot tell a rename
apart: the caller
+ * looked the metalake up by name, so a renamed row means the name in the
request no longer
+ * exists. The metalake version is deliberately not compared, matching
{@code CatalogMetaService}:
+ * holding the row is what makes the create safe, and an unrelated metalake
edit that commits in
+ * between would otherwise reject the create for no reason.
+ */
+ private void lockMetalakeForPolicyCreate(MetalakePO observedMetalakePO) {
Review Comment:
Same conclusion as the previous round — see the reply at
https://github.com/apache/gravitino/pull/12782#discussion_r3914477714.
The reusable mechanism is already centralised in
`OccWriteSupport.lockParentForChildWrite`. This private adapter only supplies
the policy service's mapper lookup and identity predicate, and catalog, group,
role and user all deliberately keep their own service-local adapter for the
same reason. Moving `MetalakeMetaMapper` and `SessionUtils` into the generic
helper would couple it to one parent type and lower its abstraction level.
##########
core/src/main/java/org/apache/gravitino/storage/relational/service/PolicyMetaService.java:
##########
@@ -143,69 +152,87 @@ public <E extends Entity & HasIdentifier> PolicyEntity
updatePolicy(
updatedPolicyEntity.id(),
oldPolicyEntity.id());
- Integer updateResult;
try {
- boolean checkNeedUpdateVersion =
- POConverters.checkPolicyVersionNeedUpdate(
- oldPolicyPO.getPolicyVersionPO(), updatedPolicyEntity);
PolicyPO newPolicyPO =
- POConverters.updatePolicyPOWithVersion(
- oldPolicyPO, updatedPolicyEntity, checkNeedUpdateVersion);
- if (checkNeedUpdateVersion) {
- SessionUtils.doMultipleWithCommit(
- () ->
- SessionUtils.doWithoutCommit(
- PolicyVersionMapper.class,
- mapper ->
mapper.insertPolicyVersion(newPolicyPO.getPolicyVersionPO())),
- () ->
- SessionUtils.doWithoutCommit(
- PolicyMetaMapper.class,
- mapper -> mapper.updatePolicyMeta(newPolicyPO,
oldPolicyPO)));
- // we set the updateResult to 1 to indicate that the update is
successful
- updateResult = 1;
- } else {
- updateResult =
- SessionUtils.doWithCommitAndFetchResult(
- PolicyMetaMapper.class,
- mapper -> mapper.updatePolicyMeta(newPolicyPO, oldPolicyPO));
- }
+ POConverters.updatePolicyPOWithVersion(oldPolicyPO,
updatedPolicyEntity);
+ SessionUtils.doMultipleWithCommit(
+ () -> updatePolicyRootWithVersion(ident, oldPolicyPO, newPolicyPO),
+ () ->
+ SessionUtils.doWithoutCommit(
+ PolicyVersionMapper.class,
+ mapper ->
mapper.insertPolicyVersion(newPolicyPO.getPolicyVersionPO())));
} catch (RuntimeException re) {
ExceptionUtils.checkSQLException(
re, Entity.EntityType.POLICY,
updatedPolicyEntity.nameIdentifier().toString());
throw re;
}
- if (updateResult > 0) {
- return updatedPolicyEntity;
- } else {
- throw new IOException("Failed to update the entity: " +
updatedPolicyEntity);
- }
+ return updatedPolicyEntity;
}
@Monitored(
metricsSource = GRAVITINO_RELATIONAL_STORE_METRIC_NAME,
baseMetricName = "deletePolicy")
public boolean deletePolicy(NameIdentifier ident) {
Review Comment:
Accurate, but declining.
The CAS needs the version the caller observed, so the row has to be read
before the delete — that is the point of the change. Narrowing the read to
`policy_id` + `current_version` means a new mapper method plus base and
PostgreSQL SQL-provider entries, which is more of the duplicated-SQL surface
that other comments in this round are flagging. Delete is not a hot path.
Worth revisiting if profiling on large policy content shows it matters.
##########
core/src/main/java/org/apache/gravitino/storage/relational/service/PolicyMetaService.java:
##########
@@ -440,6 +475,217 @@ public int deletePolicyVersionsByRetentionCount(Long
versionRetentionCount, int
return totalDeletedCount;
}
+ /**
+ * Holds the parent metalake row for the rest of the transaction, so a
policy cannot be created
+ * under a metalake that is going away.
+ *
+ * <p>The lock is shared, not exclusive: many policies can be created under
the same metalake at
+ * the same time, while dropping the metalake takes an exclusive lock on
this row, so a drop and a
+ * create cannot overlap.
+ *
+ * <p>The name is compared again because the ID alone cannot tell a rename
apart: the caller
+ * looked the metalake up by name, so a renamed row means the name in the
request no longer
+ * exists. The metalake version is deliberately not compared, matching
{@code CatalogMetaService}:
+ * holding the row is what makes the create safe, and an unrelated metalake
edit that commits in
+ * between would otherwise reject the create for no reason.
+ */
+ private void lockMetalakeForPolicyCreate(MetalakePO observedMetalakePO) {
+ OccWriteSupport.lockParentForChildWrite(
+ observedMetalakePO.getMetalakeName(),
+ Entity.EntityType.METALAKE,
+ () ->
+ SessionUtils.getWithoutCommit(
+ MetalakeMetaMapper.class,
+ mapper ->
+
mapper.selectMetalakeMetaByIdForShare(observedMetalakePO.getMetalakeId())),
+ null,
+ current -> Objects.equals(current.getMetalakeName(),
observedMetalakePO.getMetalakeName()));
+ }
+
+ /**
+ * Writes the policy row and its content snapshot.
+ *
+ * <p>An overwrite is not an upsert any more: the existing row is located
and locked first, and
+ * the replacement is written as the next version of that row, so the
snapshot history survives
+ * the overwrite instead of being reset. When no row is there to replace,
the overwrite inserts
+ * like a plain create. If another create wins the unique key after the
locking lookup misses, the
+ * caller receives a retryable optimistic-lock failure after the transaction
is rolled back.
+ *
+ * <p>An overwrite no longer revives a soft-deleted row that happens to
carry the same policy ID.
+ * Such a row keeps the primary key, so the insert is rejected as an
already-existing policy,
+ * which is the honest answer: the snapshots and relations of the deleted
policy are gone with it.
+ */
+ private void insertPolicyWithoutCommit(
+ PolicyEntity policyEntity, PolicyPO initializedPolicyPO, boolean
overwritten) {
+ if (!overwritten) {
+ insertNewPolicyWithoutCommit(initializedPolicyPO);
+ return;
+ }
+
+ PolicyPO existingPolicyPO =
findAndLockPolicyForOverwrite(initializedPolicyPO);
+ if (existingPolicyPO == null) {
+ if (SessionUtils.getWithoutCommit(
+ PolicyMetaMapper.class,
+ mapper ->
mapper.countDeletedPolicyMetasById(initializedPolicyPO.getPolicyId()))
Review Comment:
Declining. `PolicyManager.java:165` is the only production writer of a
`PolicyEntity` and always passes `overwritten = false`, so the overwrite-miss
path is reachable from tests only. Folding the `deleted_at` check into the
previous query would optimise three round trips that no production caller
makes, in exchange for moving a filter from SQL into Java on a path that
currently has an easy-to-follow shape.
##########
core/src/main/java/org/apache/gravitino/storage/relational/service/PolicyMetaService.java:
##########
@@ -440,6 +475,217 @@ public int deletePolicyVersionsByRetentionCount(Long
versionRetentionCount, int
return totalDeletedCount;
}
+ /**
+ * Holds the parent metalake row for the rest of the transaction, so a
policy cannot be created
+ * under a metalake that is going away.
+ *
+ * <p>The lock is shared, not exclusive: many policies can be created under
the same metalake at
+ * the same time, while dropping the metalake takes an exclusive lock on
this row, so a drop and a
+ * create cannot overlap.
+ *
+ * <p>The name is compared again because the ID alone cannot tell a rename
apart: the caller
+ * looked the metalake up by name, so a renamed row means the name in the
request no longer
+ * exists. The metalake version is deliberately not compared, matching
{@code CatalogMetaService}:
+ * holding the row is what makes the create safe, and an unrelated metalake
edit that commits in
+ * between would otherwise reject the create for no reason.
+ */
+ private void lockMetalakeForPolicyCreate(MetalakePO observedMetalakePO) {
+ OccWriteSupport.lockParentForChildWrite(
+ observedMetalakePO.getMetalakeName(),
+ Entity.EntityType.METALAKE,
+ () ->
+ SessionUtils.getWithoutCommit(
+ MetalakeMetaMapper.class,
+ mapper ->
+
mapper.selectMetalakeMetaByIdForShare(observedMetalakePO.getMetalakeId())),
+ null,
+ current -> Objects.equals(current.getMetalakeName(),
observedMetalakePO.getMetalakeName()));
+ }
+
+ /**
+ * Writes the policy row and its content snapshot.
+ *
+ * <p>An overwrite is not an upsert any more: the existing row is located
and locked first, and
+ * the replacement is written as the next version of that row, so the
snapshot history survives
+ * the overwrite instead of being reset. When no row is there to replace,
the overwrite inserts
+ * like a plain create. If another create wins the unique key after the
locking lookup misses, the
+ * caller receives a retryable optimistic-lock failure after the transaction
is rolled back.
+ *
+ * <p>An overwrite no longer revives a soft-deleted row that happens to
carry the same policy ID.
+ * Such a row keeps the primary key, so the insert is rejected as an
already-existing policy,
+ * which is the honest answer: the snapshots and relations of the deleted
policy are gone with it.
+ */
+ private void insertPolicyWithoutCommit(
+ PolicyEntity policyEntity, PolicyPO initializedPolicyPO, boolean
overwritten) {
+ if (!overwritten) {
+ insertNewPolicyWithoutCommit(initializedPolicyPO);
+ return;
+ }
+
+ PolicyPO existingPolicyPO =
findAndLockPolicyForOverwrite(initializedPolicyPO);
+ if (existingPolicyPO == null) {
+ if (SessionUtils.getWithoutCommit(
+ PolicyMetaMapper.class,
+ mapper ->
mapper.countDeletedPolicyMetasById(initializedPolicyPO.getPolicyId()))
+ > 0) {
+ throw new EntityAlreadyExistsException(
Review Comment:
Declining for now. There is one user of this classification today, so the
right shape of the abstraction is not yet visible; generalising
`OccWriteSupport` around a single call site tends to produce a helper that the
second user then has to fight. When a second entity type needs the same "ID
reserved by a deleted row" or "cross-parent ID reuse" checks, that is the point
to lift it.
--
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]
