lasdf1234 commented on code in PR #12420:
URL: https://github.com/apache/gravitino/pull/12420#discussion_r3767336081
##########
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:
Fixed: rollback now runs only when catalog `createSchema` did not succeed
(`AtomicBoolean`). Write-through URNs are entityId-scoped, so a failed recreate
no longer risks deleting secrets still referenced by an existing schema. Added
a regression assertion in `testCreateWithSecrets`.
--
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]
