jerryshao commented on code in PR #13367:
URL: https://github.com/apache/gravitino/pull/13367#discussion_r4061219361
##########
core/src/main/java/org/apache/gravitino/GravitinoEnv.java:
##########
@@ -229,24 +231,43 @@ public static GravitinoEnv getInstance() {
*/
public void initializeBaseComponents(Config config) {
LOG.info("Initializing Gravitino base environment...");
- this.config = config;
- FileFetcher.get().initialize(config.get(Configs.BLOCK_UNSAFE_REMOTE_URI));
- SecretPropertyUtils.configureSensitiveKeyKeywords(config);
+ initializeConfig(config);
this.manageFullComponents = false;
initBaseComponents();
LOG.info("Gravitino base environment is initialized.");
}
+ /**
+ * Initializes components required for normalized metadata operations.
+ *
+ * <p>This initialization profile does not initialize event listeners, audit
logging, metadata
+ * hooks, auxiliary services, or job management.
+ *
+ * <p>This method must be called on {@link #getInstance()}. Some metadata
components read their
+ * dependencies directly from that singleton instead of from the object
being initialized.
+ *
+ * @param config The configuration object to initialize the environment.
+ */
+ public void initializeMetadataComponents(Config config) {
+ Preconditions.checkState(
+ this == getInstance(),
+ "Metadata components must be initialized on
GravitinoEnv.getInstance().");
+ LOG.info("Initializing Gravitino metadata environment...");
+ initializeConfig(config);
+ this.manageFullComponents = false;
+ initCommonComponents();
+ initMetadataComponents();
Review Comment:
[Question] With re-initialization explicitly out of scope, is it worth
saying on this method (or on `shutdown()`) that an env is initialized once per
process and that `shutdown()` does not return it to a reusable state?
The concrete case is the `LockManager` this profile creates at
`GravitinoEnv.java:1038`. Its constructor starts a node cleaner and a deadlock
checker
(`core/src/main/java/org/apache/gravitino/lock/LockManager.java:183-194`), each
on a `ScheduledThreadPoolExecutor` held only in a local variable
(`LockManager.java:105-112`, `152-159`), and the class exposes no stop or close
method -- only the constructor and `createTreeLock` are public. `shutdown()`
(`GravitinoEnv.java:802-864`) closes the entity store, catalog manager, metrics
system, metalake manager, job dispatcher, statistic dispatcher, KMS registry
and secret manager, but has no branch for `lockManager`, so those two threads
survive it.
For the server this never mattered: one init, then the process exits. This
profile is advertised for embedded hosts that keep running after `shutdown()`,
and each `initializeMetadataComponents` call adds another pair. They are daemon
threads firing once every 60s / `cleanTreeNodeIntervalInSecs`, so the cost is
small, and this is pre-existing behaviour rather than something the PR
introduces -- but the new test class already builds three of them in one JVM,
which is exactly the repeated-init shape the new profile invites.
Verified by: read `shutdown()`, `initEntityStoreAndCatalogManager()` and the
`LockManager` constructor on this branch; grepped `LockManager` for a
close/stop entry point and found none.
##########
core/src/test/java/org/apache/gravitino/TestGravitinoEnvMetadataComponents.java:
##########
@@ -0,0 +1,336 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.gravitino;
+
+import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertInstanceOf;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertNull;
+import static org.junit.jupiter.api.Assertions.assertSame;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.mockito.Mockito.clearInvocations;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.mockStatic;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.withSettings;
+
+import java.lang.reflect.Field;
+import java.lang.reflect.Modifier;
+import java.util.Collections;
+import java.util.LinkedHashMap;
+import java.util.Map;
+import org.apache.commons.lang3.reflect.FieldUtils;
+import org.apache.gravitino.Entity.EntityType;
+import org.apache.gravitino.catalog.FilesetNormalizeDispatcher;
+import org.apache.gravitino.catalog.FilesetOperationDispatcher;
+import org.apache.gravitino.catalog.FunctionNormalizeDispatcher;
+import org.apache.gravitino.catalog.FunctionOperationDispatcher;
+import org.apache.gravitino.catalog.ModelNormalizeDispatcher;
+import org.apache.gravitino.catalog.ModelOperationDispatcher;
+import org.apache.gravitino.catalog.PartitionNormalizeDispatcher;
+import org.apache.gravitino.catalog.PartitionOperationDispatcher;
+import org.apache.gravitino.catalog.SchemaNormalizeDispatcher;
+import org.apache.gravitino.catalog.SchemaOperationDispatcher;
+import org.apache.gravitino.catalog.TableNormalizeDispatcher;
+import org.apache.gravitino.catalog.TableOperationDispatcher;
+import org.apache.gravitino.catalog.TopicNormalizeDispatcher;
+import org.apache.gravitino.catalog.TopicOperationDispatcher;
+import org.apache.gravitino.catalog.ViewNormalizeDispatcher;
+import org.apache.gravitino.catalog.ViewOperationDispatcher;
+import org.apache.gravitino.hook.FilesetHookDispatcher;
+import org.apache.gravitino.hook.FunctionHookDispatcher;
+import org.apache.gravitino.hook.ModelHookDispatcher;
+import org.apache.gravitino.hook.SchemaHookDispatcher;
+import org.apache.gravitino.hook.TableHookDispatcher;
+import org.apache.gravitino.hook.TopicHookDispatcher;
+import org.apache.gravitino.hook.ViewHookDispatcher;
+import org.apache.gravitino.listener.FilesetEventDispatcher;
+import org.apache.gravitino.listener.FunctionEventDispatcher;
+import org.apache.gravitino.listener.ModelEventDispatcher;
+import org.apache.gravitino.listener.PartitionEventDispatcher;
+import org.apache.gravitino.listener.SchemaEventDispatcher;
+import org.apache.gravitino.listener.StatisticEventDispatcher;
+import org.apache.gravitino.listener.TableEventDispatcher;
+import org.apache.gravitino.listener.TopicEventDispatcher;
+import org.apache.gravitino.listener.ViewEventDispatcher;
+import org.apache.gravitino.meta.BaseMetalake;
+import org.apache.gravitino.stats.StatisticManager;
+import org.apache.gravitino.stats.storage.MemoryPartitionStatsStorageFactory;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.mockito.MockedStatic;
+
+class TestGravitinoEnvMetadataComponents {
+
+ private Map<Field, Object> singletonState;
+
+ @BeforeEach
+ void isolateSingletonState() throws IllegalAccessException {
+ GravitinoEnv singleton = GravitinoEnv.getInstance();
+ singletonState = snapshotState(singleton);
+ restoreState(singleton, snapshotState(new TestGravitinoEnv()));
Review Comment:
[Nit] The snapshot covers `GravitinoEnv`'s instance fields, but the
initialization paths under test also mutate two process-global singletons
through `initializeConfig` (`GravitinoEnv.java:879-883`):
`FileFetcher.get().initialize(...)` writes a volatile flag on a static holder
instance
(`common/src/main/java/org/apache/gravitino/utils/FileFetcher.java:52,56-74`),
and `SecretPropertyUtils.configureSensitiveKeyKeywords(config)` replaces a
static keyword set
(`core/src/main/java/org/apache/gravitino/secret/SecretPropertyUtils.java:59-61`
-> `SensitivePropertyKeyMatcher.java:43,65-76`). Neither is covered by
`snapshotState`, so the class leaves both configured from `metadataConfig(...)`.
Nothing breaks today: `metadataConfig` sets neither key, and both defaults
equal the values already in place -- `Configs.SENSITIVE_KEY_KEYWORDS` defaults
to `SensitivePropertyKeyKeywords.defaultKeywords()`, the same set
`SensitivePropertyKeyMatcher` starts with (`Configs.java:641-658`). It turns
into a real leak the first time someone adds `SENSITIVE_KEY_KEYWORDS` or
`BLOCK_UNSAFE_REMOTE_URI` to the test config. A restore of those two in
`@AfterEach`, or a one-line comment that they are deliberately left at their
defaults, would keep the "leaves the env as it found it" property honest.
Verified by: read `initializeConfig`, `FileFetcher.initialize`,
`SensitivePropertyKeyMatcher.configure` and the `SENSITIVE_KEY_KEYWORDS`
default on this branch.
--
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]
