bitflicker64 commented on code in PR #3220:
URL: https://github.com/apache/hugegraph/pull/3220#discussion_r4049698177
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/StandardHugeGraph.java:
##########
@@ -263,9 +263,13 @@ public StandardHugeGraph(HugeConfig config) {
throw new HugeException(message);
}
- if (isHstore()) {
+ if (isHstore() && !MetaManager.instance().isReady()) {
Review Comment:
🧹 Minor. With this guard, the WARN added to `MetaManager.connect()` never
fires for a graph, so a conflicting `pd.cluster` is dropped without any message.
`pd.cluster` is a per-graph option, but the prefix is bound once per
process. With `usePD=false` and two hstore graphs in `conf/graphs`, say one
with `pd.cluster=a` and one with `pd.cluster=b`, the second graph sees
`isReady() == true`, skips `connect()`, and reads and writes under
`HUGEGRAPH/a/` without a log line. That is the same silent empty schema this PR
fixes, just triggered by the new option. Before this change the literal `hg`
made a conflict impossible.
Could you add an `else` branch that warns (or fails) when
`config.containsKey(CoreOptions.PD_CLUSTER.name())` and the value differs from
`MetaManager.instance().cluster()`? The `containsKey` check keeps it quiet for
usePD=true graphs that leave the default. Please also say in the `PD_CLUSTER`
description that the value is process-wide and the first hstore graph opened
wins.
##########
hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/unit/core/MetaManagerClusterTest.java:
##########
@@ -0,0 +1,144 @@
+/*
+ * 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.hugegraph.unit.core;
+
+import java.lang.reflect.Field;
+
+import org.apache.commons.configuration2.PropertiesConfiguration;
+import org.apache.hugegraph.config.CoreOptions;
+import org.apache.hugegraph.config.HugeConfig;
+import org.apache.hugegraph.config.ServerOptions;
+import org.apache.hugegraph.core.GraphManager;
+import org.apache.hugegraph.meta.MetaDriver;
+import org.apache.hugegraph.meta.MetaManager;
+import org.apache.hugegraph.testutil.Assert;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Test;
+import org.mockito.Mockito;
+
+/**
+ * The meta keys in PD are prefixed with the cluster name, and
+ * MetaManager.connect() binds that name once per process. A server that
+ * connects under the configured 'cluster' must not be preceded by a graph
+ * connecting under its own 'pd.cluster', otherwise every later lookup reads
+ * an empty tree (1.7.0 wrote the schema under 'hg-test', master looked
+ * under 'hg').
+ */
+public class MetaManagerClusterTest {
+
+ private Object originalDriver;
+ private Object originalCluster;
+
+ @Before
+ public void setup() throws Exception {
+ this.originalDriver = swapField("metaDriver", null);
+ this.originalCluster = swapField("cluster", null);
+ }
+
+ @After
+ public void teardown() throws Exception {
+ swapField("metaDriver", this.originalDriver);
Review Comment:
🧹 Minor. The teardown restores `metaDriver` and `cluster`, but every
`connect()` in these tests also runs `initManagers()`, which replaces the ten
sub-managers (`authMetaManager` through `lockMetaManager`) with ones bound to
the Mockito driver and the test cluster. After this class runs, the
`MetaManager` singleton reports `isReady() == false` but its managers point at
a mock. Any later test in `UnitTestSuite` that reaches a manager without
connecting gets a silent mock instead of the NPE it would get today.
`GraphManagerAdminInitTest` in the same package already restores each
manager field it swaps. Please snapshot and restore the sub-manager fields too,
for example by iterating the non-static declared fields of `MetaManager` in
`setup()`/`teardown()`, so the suite leaves the singleton as it found it.
##########
hugegraph-server/hugegraph-dist/src/main/java/org/apache/hugegraph/dist/HugeGraphServer.java:
##########
@@ -68,6 +69,15 @@ public HugeGraphServer(String gremlinServerConf, String
restServerConf)
ServiceConstant.SERVICE_NAME,
ServiceConstant.AUTHORITY);
+ // Bind the meta cluster name ('cluster' in rest-server.properties)
+ // before any graph is opened: prepare() below opens every graph
+ // in conf/graphs, and an hstore graph would otherwise connect the
+ // MetaManager first under its own 'pd.cluster' (default 'hg'),
+ // hiding the meta written under the configured cluster
+ if (restServerConfig.get(ServerOptions.USE_PD)) {
+ GraphManager.connectMetaManager(restServerConfig);
Review Comment:
🧹 Minor. This moves the prefix back to `hg-test` for every usePD=true server
that has been running a master build since #3008 (merged 2026-07-10). Those
servers wrote their schema, graph spaces and users under `HUGEGRAPH/hg/`. After
this change they come up with an empty schema, and `ensureCluster` passes, so
the log gives no hint. `docker/docker-compose-hstore.yml:92` sets
`HG_SERVER_USE_PD: "true"` and no `HG_SERVER_CLUSTER`, so anyone on the compose
setup from master gets the `hg-test` default (`ServerOptions.CLUSTER`).
Restoring 1.7.0 behaviour is the right call, and the migration tool is fine
as a follow-up. Could you log the bound prefix at INFO here (for example `Meta
cluster bound to '{}' (keys under HUGEGRAPH/{}/)`) and add a line to the
upgrade notes for snapshot users? Then an empty schema after upgrading shows
its cause in the first lines of the log.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
