LuciferYang commented on code in PR #13440:
URL: https://github.com/apache/gravitino/pull/13440#discussion_r4089381055
##########
core/src/main/java/org/apache/gravitino/catalog/SchemaOperationDispatcher.java:
##########
@@ -420,14 +420,16 @@ private Pair<Schema, SchemaChange[]>
alterManagedSchemaUnderLock(
existing.properties() == null
? new HashMap<>()
: new HashMap<>(existing.properties());
- Pair<SchemaChange[], List<SecretMaterial>> secretResult =
+ Pair<SchemaChange[], SecretMaterialsHolder> secretResult =
SecretAlterChanges.prepareSchemaChanges(
secretManager, currentProperties, existing.id(),
changes);
- writtenSecretMaterials.set(secretResult.getRight());
+ writtenSecretMaterials.set(secretResult.getRight().get());
+
writtenSecretMaterials.setReplacedUrns(secretResult.getRight().getReplacedUrns());
effectiveChangesHolder[0] = secretResult.getLeft();
return SchemaEntityChanges.apply(ident, existing,
secretResult.getLeft());
});
alterCommitted = true;
+ writtenSecretMaterials.deleteReplaced(secretManager);
Review Comment:
The deletion is post-commit and best-effort (it catches and logs; a failure
only orphans an already-unreferenced secret, it never dangles a live
reference), and it's on the cold alter path with a single provider delete on an
already-resolved URN, so the added tail latency is bounded. Moving it out of
the locked section is possible but would mean restructuring the commit / holder
/ rollback bookkeeping that keeps the deferred delete correct, which I didn't
think worth it for this path. Happy to move it out if you feel strongly.
--
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]
