bitflicker64 commented on code in PR #3221:
URL: https://github.com/apache/hugegraph/pull/3221#discussion_r4237664481
##########
hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/core/GraphManager.java:
##########
@@ -1267,7 +1269,9 @@ public void loadGraphsFromLocal(Map<String, String>
graphConfs) {
HugeFactory.checkGraphName(name, "rest-server.properties");
try {
this.loadGraph(name, graphConfPath);
+ this.failedGraphs.remove(name);
Review Comment:
Minor: this is the only place an entry leaves `failedGraphs`, and after
`init()` the method is called again only from `reload()` when
`graph.load_from_local_config=true` (default `false`). So a graph that fails
its startup load keeps the Server at 503 for the life of the process. In
particular, creating that graph afterwards through the graphs API
(`createGraph` -> `createGraphLocal` with `usePD=false`) registers it in
`graphs` through the `GRAPH_CREATE` listener but does not touch `failedGraphs`:
the graph is served and `/readiness` still answers `1 configured graph(s)
failed to load`.
Requested change: remove the name from `failedGraphs` when a graph of that
name is loaded or created later (for example in `createGraphLocal` after the
create succeeds), and say in the README that a failed startup load otherwise
holds until the Server restarts.
##########
hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/profile/StorageReadiness.java:
##########
@@ -0,0 +1,392 @@
+/*
+ * 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.api.profile;
+
+import java.util.ArrayList;
+import java.util.HashSet;
+import java.util.LinkedHashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.CompletableFuture;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.TimeoutException;
+import java.util.concurrent.atomic.AtomicInteger;
+import java.util.concurrent.atomic.AtomicReference;
+import java.util.stream.Collectors;
+
+import org.apache.hugegraph.HugeGraph;
+import org.apache.hugegraph.auth.HugeGraphAuthProxy;
+import org.apache.hugegraph.core.GraphManager;
+import org.apache.hugegraph.util.Log;
+import org.slf4j.Logger;
+
+import com.google.common.collect.ImmutableSet;
+
+/**
+ * Whether this server can serve graph traffic, for a readiness probe.
+ * Graphs on an embedded backend are ready as soon as the REST layer answers.
+ * Graphs on a remote backend are probed through the backend's
+ * "storage_readiness" metadata: on hstore at least one known Store answers a
+ * cheap direct call (PD is only needed until the first Store list is known),
+ * on hbase the cluster answers an admin call about one of the graph's tables.
The storage is shared by every
Review Comment:
Minor: three Javadoc blocks still describe the round 5 design. This one says
the HBase probe is "an admin call about one of the graph's tables"; the
`remoteGraphs` Javadoc (lines 156-157) says graphs with "the same HBase hosts
and namespace" share one probe, which contradicts the `configKey` Javadoc below
it and the README; `ReadinessAPI` (lines 42-43) says HBase is ready "while the
cluster answers an admin call". None of them mentions the failed-load gate. The
code checks every table of every store, probes each HBase graph on its own, and
answers 503 on a failed load.
Requested change: update the three blocks to match the README section added
in this PR.
--
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]
