bitflicker64 commented on code in PR #3185:
URL: https://github.com/apache/hugegraph/pull/3185#discussion_r3939611529
##########
hugegraph-pd/hg-pd-core/src/main/java/org/apache/hugegraph/pd/raft/RaftEngine.java:
##########
@@ -203,7 +204,101 @@ public void shutDown() {
}
public boolean isLeader() {
- return this.raftNode.isLeader(true);
+ Node node = this.raftNode;
+ return node != null && node.isLeader(true);
+ }
+
+ /**
+ * Whether this node currently knows a raft leader.
+ * <p>
+ * A follower only keeps its leader id while heartbeats keep arriving
inside the election
+ * timeout, and a leader only keeps its role while it can reach a quorum.
A non-null leader
+ * therefore means this node is part of a quorum from its own point of
view, which is the
+ * signal a readiness probe needs.
+ */
+ public boolean hasLeader() {
+ return hasLeader(this.raftNode);
+ }
+
+ private static boolean hasLeader(Node node) {
+ if (node == null) {
+ return false;
+ }
+ PeerId leader = node.getLeaderId();
+ return leader != null && !leader.isEmpty();
+ }
+
+ /**
+ * Take a consistent view of the local raft state. Every field is derived
from one
+ * {@link Node} reference and a single {@code getLeaderId()} read, so a
step-down while
+ * the view is being built cannot report a ready node that knows no leader.
+ * <p>
+ * A node is ready when it has been started, is in an active state, which
jraft's
+ * {@code State.isActive()} takes to mean leader, transferring, candidate
or follower,
+ * and sees a leader. Unlike a plain liveness check this turns false as
soon as the
+ * quorum is lost.
+ */
+ public RaftStatus getRaftStatus() {
+ Node node = this.raftNode;
+ if (node == null) {
+ return new RaftStatus(false, State.STATE_UNINITIALIZED.name(),
false);
+ }
+ State state = node.getNodeState();
+ boolean active = state != null && state.isActive();
+ return new RaftStatus(active && hasLeader(node),
+ state == null ? State.STATE_UNINITIALIZED.name()
: state.name(),
+ node.isLeader(true));
Review Comment:
Fixed in 7074440 with the suggested derivation, and the javadoc now states
the guarantee the code actually provides: one node reference, one
getNodeState() read, one getLeaderId() read, leader flag derived from that
state. Checked jraft 1.3.13 bytecode before applying: isLeader(true) is exactly
state == STATE_LEADER under the read lock, so the value is unchanged, including
reporting false during STATE_TRANSFERRING, and one lock acquisition per probe
and scrape goes away. Readiness tests 10/10 on JDK 11.
##########
hugegraph-pd/hg-pd-service/src/main/java/org/apache/hugegraph/pd/metrics/PDMetrics.java:
##########
@@ -76,7 +77,28 @@ private void registerMeters() {
Gauge.builder(PREFIX + ".terms", () -> setTerms())
.description("term of partitions in PD")
.register(registry);
+ registerRaftMeters();
+ }
+ /**
+ * Raft membership gauges so operators can alert on quorum loss. They
mirror what
+ * {@code GET /v1/ready} answers: a PD that sees no leader is outside a
quorum.
+ */
+ private void registerRaftMeters() {
+ RaftEngine raft = RaftEngine.getInstance();
+ Gauge.builder(PREFIX + ".raft.leader", () -> raft.isLeader() ? 1 : 0)
+ .description("1 if this PD is the raft leader, 0 otherwise")
+ .register(registry);
+ Gauge.builder(PREFIX + ".raft.has_leader", () -> raft.hasLeader() ? 1
: 0)
Review Comment:
Fixed in 7074440 for both gauges, hg.raft.has.leader and
hg.raft.alive.peers. Verified against a live PD built from the branch that the
Prometheus rendering is byte-identical (hg_raft_has_leader,
hg_raft_alive_peers), so the docs table and the RestApiTest assertions stand as
they are.
##########
hugegraph-server/hugegraph-dist/src/assembly/travis/test-start-hugegraph-pd.sh:
##########
@@ -111,6 +111,19 @@ wait_for_pd() {
return 1
}
+# Wait until PD readiness answers, or timeout. This is the gate the docs
recommend:
+# -f rejects the 503, the body match rejects a 200 that is an auth envelope
rather
+# than a readiness answer. No credentials, and a single-node group elects
itself.
+wait_for_pd_ready() {
+ local elapsed=0
+ while (( elapsed < STARTUP_WAIT )); do
+ curl -fsS "$PD_URL/v1/ready" 2>/dev/null | grep -q '"ready":true' &&
return 0
Review Comment:
Fixed in 7074440 with the suggested capture shape, plus a comment naming the
pipefail reason so the pipe does not creep back in. Full startup script passes
13/13 against a source-built PD, including the readiness wait.
##########
hugegraph-pd/hg-pd-test/src/main/java/org/apache/hugegraph/pd/rest/interceptor/AuthenticationConfigurerTest.java:
##########
@@ -0,0 +1,84 @@
+/*
+ * 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.pd.rest.interceptor;
+
+import java.util.List;
+
+import org.junit.Assert;
+import org.junit.Test;
+import org.springframework.util.AntPathMatcher;
+import org.springframework.util.PathMatcher;
+import org.springframework.web.servlet.config.annotation.InterceptorRegistry;
+import org.springframework.web.servlet.handler.MappedInterceptor;
+
+import static org.mockito.Mockito.mock;
+
+/**
+ * The probe endpoints have to stay outside the auth interceptor. If one of
them slips back
+ * behind it, PD answers a probe with 200 and an error envelope instead of a
readiness
+ * answer, so every healthcheck gating on the body holds forever while the
status still
+ * looks fine. That failure is silent, hence a check here rather than only in
the live
+ * REST suite.
+ */
+public class AuthenticationConfigurerTest {
+
+ private static final PathMatcher MATCHER = new AntPathMatcher();
+
+ /**
+ * {@code InterceptorRegistry.getInterceptors()} is protected, so read it
from a subclass.
+ */
+ private static final class TestRegistry extends InterceptorRegistry {
+
+ List<Object> registered() {
+ return getInterceptors();
+ }
+ }
+
+ private static MappedInterceptor authInterceptor() {
+ AuthenticationConfigurer configurer = new AuthenticationConfigurer();
Review Comment:
Fixed in f4fb4ea by removing the test. Neither relocation works here:
hg-pd-service is repackaged by spring-boot-maven-plugin, so the installed jar
keeps its classes under BOOT-INF/classes and no downstream module can compile
against them (a mvn test -am reactor uses target/classes, which is why the pd
job passed while every full install failed). And a test inside hg-pd-service
would never execute, since the CI package steps run -Dmaven.test.skip=true and
the pd profiles only run hg-pd-test suites. The contract it pinned, /v1/health
and /v1/ready needing no credentials, stays covered by the live REST tests
testHealthNeedsNoAuth and testReadyNeedsNoAuthAndReflectsRaft.
--
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]
