LuciferYang commented on code in PR #13440:
URL: https://github.com/apache/gravitino/pull/13440#discussion_r4092934291
##########
core/src/main/java/org/apache/gravitino/secret/SecretAlterChanges.java:
##########
@@ -79,19 +89,54 @@ public static Pair<CatalogChange[], List<SecretMaterial>>
prepareCatalogChanges(
out.add(CatalogChange.setProperty(c.getProperty(), value));
} else if (change instanceof CatalogChange.RemoveProperty) {
CatalogChange.RemoveProperty c = (CatalogChange.RemoveProperty)
change;
- secretManager.alterRemoveProperty(properties, "catalog", entityId,
c.getProperty());
+ secretManager.alterRemoveProperty(
+ properties, "catalog", entityId, c.getProperty(), replacedUrns);
out.add(change);
} else {
out.add(change);
}
}
- return Pair.of(out.toArray(new CatalogChange[0]), List.copyOf(written));
+ return Pair.of(
+ out.toArray(new CatalogChange[0]),
+ holderOf(written, replacedUrns, properties, originalProperties));
} catch (RuntimeException e) {
- secretManager.rollbackSecrets(written);
+ // Roll back only materials the persisted entity cannot reference. An
+ // in-batch re-bind may have rewritten a deterministic URN that the
original
+ // properties still point at, so deleting it would dangle the URN.
+ secretManager.rollbackSecrets(rollbackSafe(written, originalProperties));
throw e;
}
}
+ /**
+ * Builds the result holder, dropping collected URNs that the final
properties still reference (a
+ * later change in the same batch may have re-bound the same key).
+ */
+ private static SecretMaterialsHolder holderOf(
+ List<SecretMaterial> written,
+ List<SecretUrn> replacedUrns,
+ Map<String, String> properties,
+ Map<String, String> originalProperties) {
+ SecretMaterialsHolder holder = new SecretMaterialsHolder();
+ holder.set(List.copyOf(rollbackSafe(written, originalProperties)));
+ holder.setReplacedUrns(
+ replacedUrns.stream()
+ .filter(urn -> !properties.containsValue(urn.toString()))
+ .collect(Collectors.toList()));
Review Comment:
A duplicate URN would only cause a redundant best-effort delete of an
already-deleted URN, which is caught and logged, so it is harmless. In practice
each change replaces at most one binding, so the same URN is not expected to be
collected twice in one batch. I kept it a `List` for simplicity; I can switch
to a de-duplicating collection if you'd prefer.
--
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]
