diqiu50 commented on code in PR #12420:
URL: https://github.com/apache/gravitino/pull/12420#discussion_r3781810801
##########
core/src/main/java/org/apache/gravitino/catalog/SchemaOperationDispatcher.java:
##########
@@ -331,6 +363,21 @@ public boolean dropSchema(NameIdentifier ident, boolean
cascade) throws NonEmpty
catalogIdent,
LockType.WRITE,
() -> {
+ // Capture persisted properties (including write-through secret
URNs) before drop so we
+ // can clean provider material after a successful delete.
External-ref URNs are skipped by
+ // deleteSecretsFromProperties.
+ Map<String, String> schemaProperties = null;
+ try {
+ Schema schema =
+ doWithCatalog(
+ catalogIdent,
+ c -> c.doWithSchemaOps(s -> s.loadSchema(ident)),
+ NoSuchSchemaException.class);
+ schemaProperties = schema.properties();
Review Comment:
schema.properties() is not read properties from entity store, some catalog‘s
properties does not contain URN
##########
core/src/main/java/org/apache/gravitino/catalog/SchemaOperationDispatcher.java:
##########
@@ -103,82 +109,108 @@ public NameIdentifier[] listSchemas(Namespace namespace)
throws NoSuchCatalogExc
@Override
public Schema createSchema(NameIdentifier ident, String comment, Map<String,
String> properties)
throws NoSuchCatalogException, SchemaAlreadyExistsException {
+ return createSchema(ident, comment, properties, Collections.emptyMap(),
Collections.emptyMap());
+ }
+
+ @Override
+ public Schema createSchema(
+ NameIdentifier ident,
+ String comment,
+ Map<String, String> properties,
+ Map<String, SecretBinding> secretBindings,
+ Map<String, SecretReference> secretReferences)
+ throws NoSuchCatalogException, SchemaAlreadyExistsException {
NameIdentifier catalogIdent = getCatalogIdentifier(ident);
+ long uid = idGenerator.nextId();
+ Map<String, String> entityProperties =
+ SecretPropertyUtils.copyEntityProperties(properties, secretBindings,
secretReferences);
+ List<SecretMaterial> secretMaterials =
+ secretManager.assembleSecretMaterials(
+ properties, entityProperties, "schema", uid, secretBindings,
secretReferences);
doWithCatalog(
catalogIdent,
c ->
c.doWithPropertiesMeta(
p -> {
- validatePropertyForCreate(p.schemaPropertiesMetadata(),
properties);
+ validatePropertyForCreate(p.schemaPropertiesMetadata(),
entityProperties);
return null;
}),
IllegalArgumentException.class);
- long uid = idGenerator.nextId();
+ secretManager.writeSecrets(secretMaterials);
// Add StringIdentifier to the properties, the specific catalog will
handle this
// StringIdentifier to make sure only when the operation is successful,
the related
// SchemaEntity will be visible.
+ //
+ // Same split as CatalogManager: create/storage properties keep secret
URNs. Connectors that
+ // need plaintext for runtime (e.g. Fileset FS) resolve at the conf
boundary — see
+ // FilesetCatalogOperations.mergeUpLevelConfigurations /
CatalogManager.createBaseCatalog.
StringIdentifier stringId = StringIdentifier.fromId(uid);
Map<String, String> updatedProperties =
- StringIdentifier.newPropertiesWithId(stringId, properties);
-
- return TreeLockUtils.doWithTreeLock(
- catalogIdent,
- LockType.WRITE,
- () -> {
- // we do not retrieve the schema again (to obtain some values
generated by underlying
- // catalog)
- // since some catalogs' API is async and the schema may not be
created immediately
- Schema schema =
- doWithCatalog(
- catalogIdent,
- c -> c.doWithSchemaOps(s -> s.createSchema(ident, comment,
updatedProperties)),
- NoSuchCatalogException.class,
- SchemaAlreadyExistsException.class);
+ StringIdentifier.newPropertiesWithId(stringId, entityProperties);
- // If the Schema is maintained by the Gravitino's store, we don't
have to store again.
- boolean isManagedSchema = isManagedEntity(catalogIdent,
Capability.Scope.SCHEMA);
- if (isManagedSchema) {
- return EntityCombinedSchema.of(schema)
- .withHiddenProperties(
- getHiddenPropertyNames(
- catalogIdent,
- HasPropertyMetadata::schemaPropertiesMetadata,
- schema.properties()));
- }
+ try {
+ return TreeLockUtils.doWithTreeLock(
+ catalogIdent,
+ LockType.WRITE,
+ () -> {
+ // we do not retrieve the schema again (to obtain some values
generated by underlying
+ // catalog)
+ // since some catalogs' API is async and the schema may not be
created immediately
+ Schema schema =
+ doWithCatalog(
+ catalogIdent,
+ c -> c.doWithSchemaOps(s -> s.createSchema(ident, comment,
updatedProperties)),
+ NoSuchCatalogException.class,
+ SchemaAlreadyExistsException.class);
+
+ // If the Schema is maintained by the Gravitino's store, we don't
have to store again.
+ boolean isManagedSchema = isManagedEntity(catalogIdent,
Capability.Scope.SCHEMA);
+ if (isManagedSchema) {
+ return EntityCombinedSchema.of(schema)
+ .withHiddenProperties(
+ getHiddenPropertyNames(
+ catalogIdent,
+ HasPropertyMetadata::schemaPropertiesMetadata,
+ schema.properties()));
+ }
- SchemaEntity schemaEntity =
- SchemaEntity.builder()
- .withId(uid)
- .withName(ident.name())
- .withNamespace(ident.namespace())
- .withAuditInfo(
- AuditInfo.builder()
-
.withCreator(PrincipalUtils.getCurrentPrincipal().getName())
- .withCreateTime(Instant.now())
- .build())
- .build();
+ SchemaEntity schemaEntity =
+ SchemaEntity.builder()
+ .withId(uid)
+ .withName(ident.name())
+ .withNamespace(ident.namespace())
+ .withAuditInfo(
+ AuditInfo.builder()
+
.withCreator(PrincipalUtils.getCurrentPrincipal().getName())
+ .withCreateTime(Instant.now())
+ .build())
+ .build();
+
+ try {
+ store.put(schemaEntity, true /* overwrite */);
+ } catch (Exception e) {
+ LOG.error(FormattedErrorMessages.STORE_OP_FAILURE, "put", ident,
e);
+ return EntityCombinedSchema.of(schema)
+ .withHiddenProperties(
+ getHiddenPropertyNames(
+ catalogIdent,
+ HasPropertyMetadata::schemaPropertiesMetadata,
+ schema.properties()));
+ }
- try {
- store.put(schemaEntity, true /* overwrite */);
- } catch (Exception e) {
- LOG.error(FormattedErrorMessages.STORE_OP_FAILURE, "put", ident,
e);
- return EntityCombinedSchema.of(schema)
+ // Merge both the metadata from catalog operation and the metadata
from entity store.
+ return EntityCombinedSchema.of(schema, schemaEntity)
.withHiddenProperties(
getHiddenPropertyNames(
catalogIdent,
HasPropertyMetadata::schemaPropertiesMetadata,
schema.properties()));
- }
-
- // Merge both the metadata from catalog operation and the metadata
from entity store.
- return EntityCombinedSchema.of(schema, schemaEntity)
- .withHiddenProperties(
- getHiddenPropertyNames(
- catalogIdent,
- HasPropertyMetadata::schemaPropertiesMetadata,
- schema.properties()));
- });
+ });
+ } catch (RuntimeException e) {
+ secretManager.rollbackSecrets(secretMaterials);
Review Comment:
The concern isn't sharing between schemas but liveness: the catch also
covers the code after s.createSchema() has already succeeded (isManagedEntity,
getHiddenPropertyNames), so a failure there deletes the secret while the schema
still exists in the underlying catalog holding that URN — suggest rolling back
only when the schema was not created.
##########
core/src/main/java/org/apache/gravitino/catalog/CatalogManager.java:
##########
@@ -992,6 +1049,51 @@ public boolean dropCatalog(NameIdentifier ident, boolean
force)
});
}
+ /**
+ * Deletes write-through secrets for a schema and its fileset children using
properties snapped
+ * before the entities were dropped. Entity drop must succeed before calling
this.
+ */
+ private void deleteSecretsFromPropertySnapshots(
Review Comment:
The logic is duplicate with SchemaOperationDispatcher.java
snapshotFilesetProperties and deleteSecretsAfterSchemaDrop
##########
core/src/main/java/org/apache/gravitino/catalog/SchemaOperationDispatcher.java:
##########
Review Comment:
alterSchema will remove the persisted properties.
--
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]
