jerryshao commented on code in PR #13367:
URL: https://github.com/apache/gravitino/pull/13367#discussion_r4060110182
##########
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 invoked on the singleton returned by {@link
#getInstance()}, because
+ * metadata components use environment-scoped dependencies.
+ *
+ * @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().");
Review Comment:
[Question] This guard establishes that there is exactly one env, but nothing
checks whether that env is *already* initialized. Called on a singleton that
has already run `initializeFullComponents` (or this method), it silently
overwrites `entityStore`, `catalogManager`, `lockManager` and `idGenerator`
without shutting the previous ones down -- `initEntityStoreAndCatalogManager()`
(`GravitinoEnv.java:1031-1043`) assigns unconditionally, and no init path nulls
or closes what was there.
For the server that is harmless because startup happens once, but this
profile is advertised for embedded hosts that may already hold an env. While
you are formalizing the contract, is a second `checkState` (e.g. `entityStore
== null`) or an explicitly documented re-init policy worth adding?
Verified by: read `initializeBaseComponents`,
`initializeMetadataComponents`, `initializeFullComponents` and
`initEntityStoreAndCatalogManager` on this branch.
##########
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 invoked on the singleton returned by {@link
#getInstance()}, because
+ * metadata components use environment-scoped dependencies.
Review Comment:
[Nit] The stated reason reads backwards. "environment-scoped dependencies"
suggests the components are scoped to *this* env, which would be an argument
for allowing any instance. The actual reason is the opposite: several
components resolve their dependencies through the global
`GravitinoEnv.getInstance()` instead of the env they were constructed on --
`lock/TreeLockUtils.java:46`, `catalog/TopicOperationDispatcher.java:106,142`,
`metalake/MetalakeManager.java:573`,
`storage/relational/service/SchemaMetaService.java:660`.
Something like "... because some metadata components resolve their
dependencies through `GravitinoEnv.getInstance()` rather than through this
instance" would tell the next reader why the precondition on the line below
exists.
Verified by: read each of the four call sites on this branch.
##########
core/src/test/java/org/apache/gravitino/TestGravitinoEnvMetadataComponents.java:
##########
@@ -0,0 +1,299 @@
+/*
+ * 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.util.Collections;
+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.MethodOrderer.OrderAnnotation;
+import org.junit.jupiter.api.Order;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.TestMethodOrder;
+import org.mockito.MockedStatic;
+
+@TestMethodOrder(OrderAnnotation.class)
+class TestGravitinoEnvMetadataComponents {
+
+ @Test
+ @Order(1)
+ void testMetadataProfileRejectsNonSingletonEnvironment() {
+ IllegalStateException exception =
+ assertThrows(
+ IllegalStateException.class,
+ () -> new
TestGravitinoEnv().initializeMetadataComponents(metadataConfig(false)));
+
+ assertEquals(
+ "Metadata components must be initialized on
GravitinoEnv.getInstance().",
+ exception.getMessage());
+ }
+
+ @Test
+ @Order(2)
+ void
testMetadataProfileProvidesCompleteMetadataAccessWithoutServerServices() throws
Exception {
+ Config config = metadataConfig(false);
+ EntityStore entityStore = relationStore();
+ GravitinoEnv env = GravitinoEnv.getInstance();
Review Comment:
[Important] These tests now initialize and shut down the real process-wide
singleton and never put it back, so whatever this class leaves behind is
inherited by the rest of the `core` test JVM.
`shutdown()` (`GravitinoEnv.java:800-863`) closes components but nulls only
`jobOperationDispatcher`. `entityStore`, `catalogManager`, `lockManager`,
`idGenerator`, `config`, the `internal*` dispatchers and
`internalAccessControlDispatcher` all keep their values, so after this class
`GravitinoEnv.getInstance()` permanently holds a mocked `EntityStore`, a closed
`CatalogManager`/`MetricsSystem`/`SecretManager`, and this test's `Config`.
That matters because production code resolves through the singleton rather
than through the env it was built on:
`authorization/AuthorizationUtils.java:332,393,547` branch on
`GravitinoEnv.getInstance().internalAccessControlDispatcher() != null`,
`lock/TreeLockUtils.java:46` on `lockManager()`,
`metalake/MetalakeManager.java:158,573` on `entityStore()`/`catalogManager()`,
`storage/relational/service/SchemaMetaService.java:660` on `idGenerator()` (it
throws `IllegalStateException` when that is null),
`connector/BaseCatalog.java:570` on `config()`. 46 test classes under
`core/src/test` already reach into the singleton, `build.gradle.kts` sets no
`forkEvery`, so they all share one JVM, and JUnit fixes no order between
classes. Running `testMetadataProfileProvidesInternalAuthorizationWhenEnabled`
alone via `--tests` leaves every later test in that JVM seeing authorization
enabled on a half-configured env; running the whole class leaves them seeing a
real `idGenerator` where they previ
ously saw null.
I could not point at a test that fails today, so this is order-dependence
rather than a live break -- but the earlier version of this file avoided most
of it by using a `TestGravitinoEnv` subclass, and the singleton contract added
in this commit makes that no longer possible. Suggest snapshotting the
singleton's fields before
`initializeMetadataComponents`/`initializeFullComponents` and restoring them in
the `finally`/`@AfterEach`, so the class leaves the env exactly as it found it.
Verified by: read `shutdown()` and `initEntityStoreAndCatalogManager()`
(`GravitinoEnv.java:1031-1043`) on this branch; grepped
`GravitinoEnv.getInstance()` across `*/src/main` and `core/src/test`; confirmed
`build.gradle.kts` sets neither `forkEvery` nor `maxParallelForks` for the test
tasks.
##########
core/src/main/java/org/apache/gravitino/GravitinoEnv.java:
##########
@@ -739,13 +771,26 @@ public StatisticDispatcher statisticDispatcher() {
return statisticDispatcher;
}
+ /**
+ * Get the internal StatisticDispatcher associated with the Gravitino
environment.
+ *
+ * @return The internal StatisticDispatcher instance.
+ */
+ public StatisticDispatcher internalStatisticDispatcher() {
+ Preconditions.checkArgument(
+ internalStatisticDispatcher != null, "GravitinoEnv is not
initialized.");
+ return internalStatisticDispatcher;
Review Comment:
[Question] Both accessors this PR adds are still unreferenced outside this
file and the new test: a repo-wide grep for `internalPartitionDispatcher()` and
`internalStatisticDispatcher()` returns only `GravitinoEnv.java` and
`TestGravitinoEnvMetadataComponents.java:116,126,258,263`. That is defensible
if the metadata profile is meant to be driven from outside this repo, but it
does mean the public surface added here is never exercised by a real consumer.
Is the first caller close behind, or would these be better kept package-private
until one exists?
Minor inconsistency while you are here: this accessor throws
`IllegalArgumentException` when unset, while its sibling
`internalPartitionDispatcher()` (`GravitinoEnv.java:458-459`) returns `null` in
the same situation, so callers of the two internal dispatchers need two
different not-initialized checks.
Verified by: repo-wide grep for both symbols on this branch; read both
accessors.
--
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]
