lasdf1234 commented on code in PR #12366:
URL: https://github.com/apache/gravitino/pull/12366#discussion_r3734438836
##########
core/src/main/java/org/apache/gravitino/secret/SecretManager.java:
##########
@@ -87,7 +155,7 @@ public List<SecretUrn> getSecretReferenceUrns(Map<String,
SecretReference> secre
SecretProvider provider = registry.getProvider(providerName);
try {
SecretUrn urn = provider.buildReferenceUrn(key, locator.attributes());
Review Comment:
Agreed this is important, but it’s outside the scope of this PR.It can be
optimized later.
##########
core/src/main/java/org/apache/gravitino/catalog/SchemaOperationDispatcher.java:
##########
@@ -101,84 +106,99 @@ public NameIdentifier[] listSchemas(Namespace namespace)
throws NoSuchCatalogExc
* @throws SchemaAlreadyExistsException If a schema with the same identifier
already exists.
*/
@Override
- public Schema createSchema(NameIdentifier ident, String comment, Map<String,
String> properties)
+ 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);
+ List<SecretUrn> secretUrns =
+ secretManager.assembleSecretUrns(
+ 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(secretBindings, secretUrns);
// 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.
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)),
Review Comment:
Thank you very much for your review. The text has been revised. persisted =
URN, connector/runtime conf = plaintext
##########
core/src/main/java/org/apache/gravitino/catalog/CatalogManager.java:
##########
@@ -593,12 +596,20 @@ public Catalog createCatalog(
Catalog.Type type,
String provider,
String comment,
- Map<String, String> properties)
+ Map<String, String> properties,
+ Map<String, SecretBinding> secretBindings,
+ Map<String, SecretReference> secretReferences)
throws NoSuchMetalakeException, CatalogAlreadyExistsException {
NameIdentifier metalakeIdent =
NameIdentifier.of(ident.namespace().levels());
- Map<String, String> mergedConfig = buildCatalogConf(provider, properties);
+ final Map<String, String> mergedConfig = new
HashMap<>(buildCatalogConf(provider, properties));
long uid = idGenerator.nextId();
+
+ List<SecretUrn> secretUrns =
+ secretManager.assembleSecretUrns(
+ properties, mergedConfig, "catalog", uid, secretBindings,
secretReferences);
+ secretManager.writeSecrets(secretBindings, secretUrns);
Review Comment:
Thank you for your review. The sequence has been adjusted. After checking
Metalake, the writeSecrets operation will be executed.
##########
core/src/main/java/org/apache/gravitino/catalog/SchemaOperationDispatcher.java:
##########
@@ -101,84 +106,99 @@ public NameIdentifier[] listSchemas(Namespace namespace)
throws NoSuchCatalogExc
* @throws SchemaAlreadyExistsException If a schema with the same identifier
already exists.
*/
@Override
- public Schema createSchema(NameIdentifier ident, String comment, Map<String,
String> properties)
+ 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);
+ List<SecretUrn> secretUrns =
+ secretManager.assembleSecretUrns(
+ 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(secretBindings, secretUrns);
Review Comment:
Agree with your review. The schema and fileset deletion scenarios have been
added, and the secret has also been cleaned up.
##########
core/src/main/java/org/apache/gravitino/hook/SchemaHookDispatcher.java:
##########
@@ -91,7 +98,8 @@ public Schema createSchema(NameIdentifier ident, String
comment, Map<String, Str
// ancestor's owner is never overwritten.
List<NameIdentifier> newAncestors =
findMissingAncestors(normalizedIdent);
- Schema schema = dispatcher.createSchema(ident, comment, properties);
+ Schema schema =
+ dispatcher.createSchema(ident, comment, properties,
secretBindings, secretReferences);
Review Comment:
I agree with your point of view. Currently, the creation of schema and
fileset has failed, and the secret will also be rolled back.
--
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]
