Copilot commented on code in PR #10874:
URL: https://github.com/apache/gravitino/pull/10874#discussion_r3217242316
##########
core/src/main/java/org/apache/gravitino/hook/SchemaHookDispatcher.java:
##########
@@ -59,26 +67,84 @@ public NameIdentifier[] listSchemas(Namespace namespace)
throws NoSuchCatalogExc
@Override
public Schema createSchema(NameIdentifier ident, String comment, Map<String,
String> properties)
throws NoSuchCatalogException, SchemaAlreadyExistsException {
+ // Collect missing parent identifiers BEFORE the underlying call; the
catalog itself is
+ // responsible for auto-creating them. We only need to assign ownership
afterwards.
+ List<NameIdentifier> missingParents = findMissingParents(ident);
Schema schema = dispatcher.createSchema(ident, comment, properties);
- // Set the creator as the owner of the schema.
OwnerDispatcher ownerManager =
GravitinoEnv.getInstance().ownerDispatcher();
if (ownerManager != null) {
- // The inner NormalizeDispatcher case-folds the schema name based on
catalog capabilities,
- // so the entity is stored under the normalized identifier. Apply the
same normalization
- // here so the owner is attached to the same identifier the manager sees.
- NameIdentifier normalizedIdent =
- CapabilityHelpers.applyCapabilities(
- ident, Capability.Scope.SCHEMA,
GravitinoEnv.getInstance().catalogManager());
- ownerManager.setOwner(
- normalizedIdent.namespace().level(0),
- NameIdentifierUtil.toMetadataObject(normalizedIdent,
Entity.EntityType.SCHEMA),
- PrincipalUtils.getCurrentUserName(),
- Owner.Type.USER);
+ CatalogManager catalogManager =
GravitinoEnv.getInstance().catalogManager();
+ String metalake = ident.namespace().level(0);
+ String user = PrincipalUtils.getCurrentUserName();
+ // Auto-created parent schemas (hierarchical namespace) and the new
schema each need an
+ // owner; outer-to-inner order matches parent-before-child creation.
+ List<MetadataObject> toOwn = new ArrayList<>(missingParents.size() + 1);
+ for (NameIdentifier parentIdent : missingParents) {
+ NameIdentifier normalizedParent =
+ CapabilityHelpers.applyCapabilities(
+ parentIdent, Capability.Scope.SCHEMA, catalogManager);
+ toOwn.add(NameIdentifierUtil.toMetadataObject(normalizedParent,
Entity.EntityType.SCHEMA));
+ }
+ toOwn.add(
+ NameIdentifierUtil.toMetadataObject(
+ CapabilityHelpers.applyCapabilities(ident,
Capability.Scope.SCHEMA, catalogManager),
+ Entity.EntityType.SCHEMA));
+ ownerManager.setOwners(metalake, toOwn, user, Owner.Type.USER);
Review Comment:
`createSchema` unconditionally calls `ownerManager.setOwners(...)` for all
`missingParents` computed before the underlying create. If a parent schema is
created (and possibly owned) concurrently between `findMissingParents` and
`setOwners`, this can overwrite an existing owner and corrupt ownership. To
avoid that race, filter `toOwn` to only include schemas that currently have no
owner (e.g., `ownerManager.getOwner(...)` returns empty) before calling
`setOwners` (and/or re-check existence/ownership after
`dispatcher.createSchema`).
##########
core/src/test/java/org/apache/gravitino/metrics/source/TestMetricsSource.java:
##########
@@ -34,6 +42,21 @@ public class TestMetricsSource extends MetricsSource {
private static String TEST_METRICS_SOURCE = "test";
private static int gaugeValue = 0;
+ private static Config savedGravitinoConfig;
+
+ static {
+ try {
+ savedGravitinoConfig =
+ (Config) FieldUtils.readField(GravitinoEnv.getInstance(), "config",
true);
+ Config mockConfig = mock(Config.class);
+ when(mockConfig.get(Configs.METRICS_TIME_SLIDING_WINDOW_SECONDS))
+ .thenReturn(Configs.DEFAULT_METRICS_TIME_SLIDING_WINDOW_SECONDS);
+ FieldUtils.writeField(GravitinoEnv.getInstance(), "config", mockConfig,
true);
+ } catch (IllegalAccessException e) {
Review Comment:
This static initializer mutates `GravitinoEnv` global config at class-load
time, which can leak state across tests (especially under parallel test
execution) and may leave the singleton in a modified state if class
initialization fails. Prefer moving this setup into `@BeforeEach`/`@AfterEach`
(or at least `@BeforeAll`/`@AfterAll`) without a static block so the config is
always restored deterministically and only during the test lifecycle.
--
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]
