fix(server): bind the meta cluster name before any graph is opened #3220
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -22,6 +22,7 @@ | |
| import org.apache.hugegraph.config.HugeConfig; | ||
| import org.apache.hugegraph.config.ServerOptions; | ||
| import org.apache.hugegraph.constant.ServiceConstant; | ||
| import org.apache.hugegraph.core.GraphManager; | ||
| import org.apache.hugegraph.event.EventHub; | ||
| import org.apache.hugegraph.meta.MetaManager; | ||
| import org.apache.hugegraph.meta.PdMetaDriver; | ||
|
|
@@ -68,6 +69,18 @@ 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); | ||
|
Contributor
There was a problem hiding this comment. 🧹 Minor. This moves the prefix back to 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
Contributor
Author
There was a problem hiding this comment. Done in 68b616a. After |
||
| String cluster = MetaManager.instance().cluster(); | ||
| LOG.info("Meta cluster bound to '{}' (keys under HUGEGRAPH/{}/)", | ||
| cluster, cluster); | ||
| } | ||
|
|
||
| // Prepare GremlinServer (registers GRAPH_CREATE listener) BEFORE | ||
| // RestServer starts loading graphs from PD/meta. This ensures that | ||
| // graphs loaded during RestServer initialization are captured by | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,214 @@ | ||
| /* | ||
| * 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 java.lang.reflect.Modifier; | ||
| import java.util.HashMap; | ||
| import java.util.Map; | ||
|
|
||
| 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.event.EventHub; | ||
| 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 { | ||
|
|
||
| /* | ||
| * connect() also rebuilds every sub-manager of the singleton, so the | ||
| * whole instance state is snapshotted and restored, not just the driver | ||
| * and the cluster; the suite must find the singleton as it left it. | ||
| */ | ||
| private final Map<Field, Object> snapshot = new HashMap<>(); | ||
|
|
||
| @Before | ||
| public void setup() throws Exception { | ||
| for (Field f : MetaManager.class.getDeclaredFields()) { | ||
| if (Modifier.isStatic(f.getModifiers())) { | ||
| continue; | ||
| } | ||
| f.setAccessible(true); | ||
| this.snapshot.put(f, f.get(MetaManager.instance())); | ||
| } | ||
| swapField("metaDriver", null); | ||
| swapField("cluster", null); | ||
| } | ||
|
|
||
| @After | ||
| public void teardown() throws Exception { | ||
| for (Map.Entry<Field, Object> e : this.snapshot.entrySet()) { | ||
| e.getKey().set(MetaManager.instance(), e.getValue()); | ||
| } | ||
| } | ||
|
|
||
| @Test | ||
| public void testFirstConnectWinsAndLaterNamesAreIgnored() throws Exception { | ||
| connectWithMockDriver("hg-test"); | ||
| Assert.assertEquals("hg-test", MetaManager.instance().cluster()); | ||
|
|
||
| // the graph-level fallback ('pd.cluster', default 'hg') is a no-op | ||
| MetaManager.instance().connect("hg", MetaManager.MetaDriverType.PD, | ||
| "ca", "ca", "ca", "127.0.0.1:8686"); | ||
| Assert.assertEquals("hg-test", MetaManager.instance().cluster()); | ||
| MetaManager.instance().ensureCluster("hg-test"); | ||
| } | ||
|
|
||
| @Test | ||
| public void testEnsureClusterFailsOnAnotherName() throws Exception { | ||
| connectWithMockDriver("hg"); | ||
|
|
||
| Assert.assertThrows(IllegalStateException.class, () -> { | ||
| MetaManager.instance().ensureCluster("hg-test"); | ||
| }, e -> { | ||
| Assert.assertContains("connected to cluster 'hg'", e.getMessage()); | ||
| Assert.assertContains("configured cluster is 'hg-test'", | ||
| e.getMessage()); | ||
| Assert.assertContains("HUGEGRAPH/hg/", e.getMessage()); | ||
| }); | ||
| } | ||
|
|
||
| @Test | ||
| public void testEnsureClusterRequiresAConnection() { | ||
| Assert.assertThrows(IllegalStateException.class, () -> { | ||
| MetaManager.instance().ensureCluster("hg-test"); | ||
| }); | ||
| } | ||
|
|
||
| /** | ||
| * The server binds the configured cluster before any graph is opened; | ||
| * a graph that got there first under another name is a startup error, | ||
| * not a silently empty schema. | ||
| */ | ||
| @Test | ||
| public void testServerConnectDetectsGraphLevelCluster() throws Exception { | ||
| connectWithMockDriver(CoreOptions.PD_CLUSTER.defaultValue()); | ||
|
|
||
| HugeConfig conf = serverConfig(true); | ||
| Assert.assertEquals("hg-test", conf.get(ServerOptions.CLUSTER)); | ||
| Assert.assertThrows(IllegalStateException.class, () -> { | ||
| GraphManager.connectMetaManager(conf); | ||
| }, e -> { | ||
| Assert.assertContains("configured cluster is 'hg-test'", | ||
| e.getMessage()); | ||
| }); | ||
| } | ||
|
|
||
| @Test | ||
| public void testServerConnectIsIdempotentUnderTheConfiguredCluster() | ||
| throws Exception { | ||
| connectWithMockDriver("hg-test"); | ||
| HugeConfig conf = serverConfig(true); | ||
| GraphManager.connectMetaManager(conf); | ||
| GraphManager.connectMetaManager(conf); | ||
| Assert.assertEquals("hg-test", MetaManager.instance().cluster()); | ||
| } | ||
|
|
||
| /** | ||
| * With usePD=false the server binds no cluster of its own: an hstore graph | ||
| * opened by HugeGremlinServer.prepare() binds the MetaManager under its | ||
| * 'pd.cluster' (default 'hg'), and nothing on the server side may check | ||
| * that binding against the server's 'cluster' default ('hg-test'). | ||
| */ | ||
| @Test | ||
| public void testUsePdFalseLeavesTheGraphLevelBindingAlone() throws Exception { | ||
| connectWithMockDriver(CoreOptions.PD_CLUSTER.defaultValue()); | ||
|
|
||
| HugeConfig conf = serverConfig(false); | ||
| Assert.assertFalse(conf.get(ServerOptions.USE_PD)); | ||
| Assert.assertEquals("hg-test", conf.get(ServerOptions.CLUSTER)); | ||
| GraphManager.connectMetaManager(conf); | ||
| Assert.assertEquals("hg", MetaManager.instance().cluster()); | ||
| } | ||
|
|
||
| /** | ||
| * The same on the real startup path: a GraphManager built with usePD=false | ||
| * after a graph already bound 'hg' must construct without touching the | ||
| * binding (initMetaManager() only runs from loadMetaFromPD(), i.e. with | ||
| * usePD=true). | ||
| */ | ||
| @Test | ||
| public void testGraphManagerStartupWithUsePdFalseKeepsTheGraphBinding() | ||
| throws Exception { | ||
| connectWithMockDriver(CoreOptions.PD_CLUSTER.defaultValue()); | ||
|
|
||
| HugeConfig conf = serverConfig(false); | ||
| GraphManager manager = new GraphManager(conf, new EventHub("cluster-test")); | ||
| try { | ||
| Assert.assertEquals("hg", MetaManager.instance().cluster()); | ||
| } finally { | ||
| manager.close(); | ||
| } | ||
| } | ||
|
|
||
| @Test | ||
| public void testTeardownRestoresTheSubManagers() throws Exception { | ||
| Field f = MetaManager.class.getDeclaredField("authMetaManager"); | ||
| f.setAccessible(true); | ||
| Object before = this.snapshot.get(f); | ||
| connectWithMockDriver("hg-test"); | ||
| Assert.assertNotSame(before, f.get(MetaManager.instance())); | ||
| teardown(); | ||
| Assert.assertSame(before, f.get(MetaManager.instance())); | ||
| } | ||
|
|
||
| @Test | ||
| public void testGraphLevelDefaultStaysHg() { | ||
| Assert.assertEquals("hg", CoreOptions.PD_CLUSTER.defaultValue()); | ||
| } | ||
|
|
||
| /** rest-server.properties as the server sees it, with usePD set explicitly. */ | ||
| private static HugeConfig serverConfig(boolean usePd) { | ||
| PropertiesConfiguration props = new PropertiesConfiguration(); | ||
| // HugeConfig.get() casts stored values, it does not parse strings here | ||
| props.setProperty(ServerOptions.USE_PD.name(), Boolean.valueOf(usePd)); | ||
| return new HugeConfig(props); | ||
| } | ||
|
|
||
| private static void connectWithMockDriver(String cluster) throws Exception { | ||
| // connect() builds a real driver, so pre-bind a mock one and the | ||
| // cluster the same way the first successful connect() would | ||
| swapField("metaDriver", Mockito.mock(MetaDriver.class)); | ||
| swapField("cluster", cluster); | ||
| MetaManager.instance().connect(cluster, MetaManager.MetaDriverType.PD, | ||
| null, null, null, "127.0.0.1:8686"); | ||
| } | ||
|
|
||
| private static void swapField(String field, Object replacement) | ||
| throws Exception { | ||
| Field f = MetaManager.class.getDeclaredField(field); | ||
| f.setAccessible(true); | ||
| f.set(MetaManager.instance(), replacement); | ||
| } | ||
| } |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
usePD=falseHStore startup,HugeGraphServerskips the pre-bind at itsusePDguard, whileHugeGremlinServer.prepare()opens the graph beforeHugeRestServer.start(); the graph then bindsMetaManagerthrough thepd.clusterfallback (defaulthg).GraphManager.initMetaManager()subsequently calls this method unconditionally, and the default serverclusterishg-test, soensureCluster(cluster)throws instead of preserving the graph-level binding. This regresses the usePD=false compatibility path described by the PR. Please gate this check to the explicit usePD=true server bind, or otherwise use the already-bound graph cluster for usePD=false, and add a startup test covering that path.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for tracing this path; I checked it two ways and would like to be precise, because in the described form it does not occur.
ensureCluster()is called only fromconnectMetaManager(), which has two callers:HugeGraphServer(explicitly behind theusePDguard) andGraphManager.initMetaManager().initMetaManager()has a single caller,loadMetaFromPD()(line 376), andloadMetaFromPD()runs only in the constructor underif (PDExist)(lines 271-274), i.e. only withusePD=true. WithusePD=falsethe server never reachesensureClusterat all;StandardHugeGraphbindspd.clusterand that is the end of it, exactly as in 1.7.0.Measured on the lab (1 PD + 3 stores, data written by the official 1.7.0 release, a build of this head), with
usePDandpd.peersremoved fromrest-server.properties: with the defaultpd.clusterthe server starts, the graph opens, 0 property keys (the data lives underhg-test, which is expected), and zeroensureCluster/IllegalStateExceptionlines in the log; withpd.cluster=hg-testin the graph file the server starts and sees all 13 keys, log equally clean. WithusePD=truerestored: 13 keys andMeta cluster bound to 'hg-test'. Log:results/upgrade-170-to-master/logs/usepd-false-round2.txtin the validation repo.I added the gate anyway in 8e914e6, since it is cheap and protects against a future caller:
connectMetaManager()returns without binding or checking whenusePD=false, with a comment that the graph-level binding is then the only one and no server-side check may apply to it. Two tests:testUsePdFalseLeavesTheGraphLevelBindingAlone(a graph boundhg, calling the helper withusePD=falsedoes not throw and the cluster stayshg) andtestGraphManagerStartupWithUsePdFalseKeepsTheGraphBinding(aGraphManagerconstructed withusePD=falseafter such a binding completes without an exception, cluster stillhg).MetaManagerClusterTest9/9.