Copilot commented on code in PR #13440:
URL: https://github.com/apache/gravitino/pull/13440#discussion_r4073220246
##########
core/src/main/java/org/apache/gravitino/secret/SecretAlterChanges.java:
##########
@@ -57,19 +59,27 @@ public static Pair<CatalogChange[], List<SecretMaterial>>
prepareCatalogChanges(
currentProperties == null ? new HashMap<>() : new
HashMap<>(currentProperties);
List<CatalogChange> out = new ArrayList<>(changes.length);
List<SecretMaterial> written = new ArrayList<>();
+ List<SecretUrn> replacedUrns = new ArrayList<>();
+ Map<String, String> originalProperties = Map.copyOf(properties);
Review Comment:
`Map.copyOf(properties)` throws `NullPointerException` if `properties`
contains any null keys/values. Since `properties` is derived from persisted
entity properties, this newly introduces a hard failure mode during prepare.
Consider taking a defensive snapshot that preserves existing null-tolerance
(e.g., `new HashMap<>(properties)` wrapped with
`Collections.unmodifiableMap(...)`) so prepare/rollback behavior doesn’t change
due to null entries.
##########
core/src/main/java/org/apache/gravitino/secret/SecretManager.java:
##########
@@ -384,7 +394,8 @@ public String alterSetSecretBinding(
long entityId,
String property,
SecretBinding binding,
- List<SecretMaterial> written) {
+ List<SecretMaterial> written,
+ List<SecretUrn> replacedUrns) {
Review Comment:
The PR changes public method signatures (`alterSetSecretBinding`,
`alterSetSecretReference`, `alterRemoveProperty`) by adding required
parameters. Because these are `public`, this is a source/binary breaking change
for any downstream callers not updated in this PR. If `SecretManager` is part
of a public surface area, add backward-compatible overloads that delegate to
the new methods (e.g., allocate an internal `List<SecretUrn>` when callers
don’t care) and deprecate the old signatures if desired.
##########
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:
`replacedUrns` can accumulate duplicates across multiple changes in the same
batch (e.g., repeated remove/set cycles on the same key), leading to redundant
deletion attempts and noisier logs if providers throw on already-deleted URNs.
Consider de-duplicating (e.g., distinct/LinkedHashSet to preserve order) before
storing them in the holder, and optionally wrapping the final list with
`List.copyOf(...)` to make the holder contents immutable like `materials`.
##########
core/src/test/java/org/apache/gravitino/secret/TestSecretManagerAlter.java:
##########
@@ -55,17 +60,33 @@ void testAlterSetSecretBindingWritesAndReturnsUrn() {
}
@Test
- void testAlterRemovePropertyDeletesWriteThroughSecret() {
+ void testAlterRemovePropertyDefersWriteThroughSecretDeletion() {
try (SecretManager secretManager = memorySecretManager()) {
Map<String, String> props = new HashMap<>();
List<SecretMaterial> written = new ArrayList<>();
+ List<SecretUrn> replacedUrns = new ArrayList<>();
String urn =
secretManager.alterSetSecretBinding(
- props, "catalog", 7L, "jdbc-password", new
SecretBinding("memory", "old"), written);
+ props,
+ "catalog",
+ 7L,
+ "jdbc-password",
+ new SecretBinding("memory", "old"),
+ written,
+ replacedUrns);
- secretManager.alterRemoveProperty(props, "catalog", 7L, "jdbc-password");
+ secretManager.alterRemoveProperty(props, "catalog", 7L, "jdbc-password",
replacedUrns);
Assertions.assertFalse(props.containsKey("jdbc-password"));
+ // Deletion is deferred until the alter commits: the removed URN must
stay
+ // resolvable while the alter may still abort.
+ Assertions.assertEquals(
+ "old",
+
secretManager.getRegistry().getProvider("memory").readSecret(SecretUrn.parse(urn)));
Review Comment:
`replacedUrns.get(0)` will throw `IndexOutOfBoundsException` if the list is
unexpectedly empty, which can obscure the real failure. It’s better to assert
the expected size first (e.g., assert size is 1) and then assert on element
contents; that yields clearer test failures.
##########
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:
This method is named `alterManagedSchemaUnderLock`, and it calls
`deleteReplaced(...)` immediately after commit. Even though deletion is
best-effort, it may still perform potentially slow I/O (provider calls) while
the lock is held, increasing tail latency and risk of lock contention. Prefer
performing post-commit deletions outside the lock/critical section (e.g., after
returning from the under-lock block) or dispatching deletion asynchronously so
the lock is held only for state mutation.
--
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]
