diqiu50 commented on code in PR #12420:
URL: https://github.com/apache/gravitino/pull/12420#discussion_r3766768205
##########
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();
Review Comment:
Schema properties are not stored in Gravitino's entity store at all; their
only source is the underlying catalog. Will they be lost for catalogs that
don't support them?
##########
core/src/main/java/org/apache/gravitino/catalog/CatalogManager.java:
##########
@@ -975,9 +1008,12 @@ public boolean dropCatalog(NameIdentifier ident, boolean
force)
// Finally, delete the catalog entity as well as all its
sub-entities from the store.
// Invalidate after store.delete() to prevent a background thread
from repopulating
// the cache with stale data between invalidate and delete.
+ Map<String, String> catalogProperties =
+ catalogWrapper.catalog().entity().getProperties();
boolean deleted = store.delete(ident, EntityType.CATALOG, true);
Review Comment:
Cascade deletion does not clean up the secrets of child entities at all.
##########
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:
If load throws any other exception, the drop operation will fail.
##########
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:
Will this delete a secret that is currently referenced by an existing schema?
--
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]
