bitflicker64 commented on PR #3185:
URL: https://github.com/apache/hugegraph/pull/3185#issuecomment-5560515804
@imbajin Ready for a review pass when you have time. Pushed `2df9a55`, five
added lines in `hugegraph-pd/docs/api-reference.md`.
**Why.** `hg_raft_alive_peers` is backed by `Node.listAlivePeers()`, so it
can only answer on the leader and returns `Double.NaN` elsewhere. Prometheus
renders that as `NaN`, and `NaN` propagates through `sum()` and `avg()`,
poisoning the aggregate. The doc gave no query form for that gauge at all,
which is the gap that leads someone to `sum()` it, so `2df9a55` adds one. I
left `PDMetrics` alone: `NaN` is the honest encoding for "this node cannot
answer", where `0` would read as "no peers alive" and poison an aggregate more
quietly.
**On the two red codecov marks.** `codecov/patch` reports one missing line,
and it is the `excludePathPatterns` registration in
`AuthenticationConfigurer.addInterceptors`. That is a Spring `WebMvcConfigurer`
callback which only runs inside the live PD process, and CI starts that process
separately. An in-JVM interceptor test was tried and reverted in `f4fb4ea`, for
a different reason worth stating accurately: `hg-pd-service` is repackaged by
`spring-boot-maven-plugin`, so a full `mvn install` leaves an executable jar
with classes under `BOOT-INF/classes` where javac cannot see them, and
`build-commons` plus both macOS server jobs failed with cannot-find-symbol. The
behaviour that line configures is covered end to end by
`testReadyNeedsNoAuthAndReflectsRaft`. `codecov/project` is a ratchet of a few
hundredths of a point, not a coverage cliff.
**What I checked on the branch.** `ProbeView` publication across all six
JRaft callbacks: `onLeaderStart`, `onLeaderStop`, `onStartFollowing`,
`onStopFollowing`, `onError`, `onShutdown`. No ordering yields a torn read,
since each announcement replaces the whole view.
`onStopFollowing` is the one place a view mixes fields from two
announcements, via `new ProbeView(this.probeView.state, false)`. The field that
would actually hurt is `isLeader`: a carried `STATE_LEADER` would serve
`{"ready":false,"state":"STATE_LEADER","isLeader":true}` and make
`hg_raft_leader` read 1 on a non-leader. That is unreachable in the jraft
1.3.13 that `hg-pd-core` pins. `onStopFollowing` is only emitted from
`resetLeaderId` under `state.compareTo(STATE_TRANSFERRING) > 0` at
`NodeImpl.java:1197`, which excludes both `STATE_LEADER` and
`STATE_TRANSFERRING`; and although `transferLeadershipTo` sets the state to
`TRANSFERRING` before firing `onLeaderStop`, `stepDown` announces
`onLeaderStop` while the state is still `LEADER`, ahead of `resetLeaderId`. So
the reachable carried states are `FOLLOWER`, `ERROR`, `SHUTDOWN` and
`UNINITIALIZED`, all of which give `ready=false` and `isLeader=false`.
`RaftEngineReadinessTest` asserts on `isReady()` in seven methods, and
`testReadyNeedsNoAuthAndReflectsRaft` asserts the body keys and the 200 path
against a live PD.
**Merge-order conflict with #3189, worth deciding before either lands.**
Both PRs rewrite the same line. This one makes it
`excludePathPatterns("/actuator/*", "/v1/health", "/v1/ready",
"/v1/prom/targets/*")`; #3189 makes it `excludePathPatterns("/actuator/**",
"/v1/health", "/v1/prom/targets/*")`. Whichever lands second conflicts, and the
two changes disagree about two independent things at once. The correct union is:
```java
excludePathPatterns("/actuator/**", "/v1/health", "/v1/ready",
"/v1/prom/targets/*")
```
Dropping `/v1/ready` puts the readiness endpoint back behind authentication
and defeats this PR; keeping the single star re-breaks the doc lines #3189
corrects.
**Two follow-ups I would not hold the merge for.**
1. The 200 versus 503 mapping in `checkReady()` has no direct test. An
inverted ternary would fail CI today, so only an "always return 200" regression
could ship undetected. Future test debt rather than a present defect, but it is
the branch most worth pinning later, given what the endpoint gates.
2. `LEADER_STOP` and `STOP_FOLLOWING` ride the same `FSMCallerImpl`
disruptor as `onApply` and `onSnapshotLoad`, so a follower in a long
`onSnapshotLoad` can keep answering 200 until the queue drains. The
`getRaftStatus` javadoc discloses this; `api-reference.md` only hints at it,
saying the state is the last change raft announced. Staleness is bounded, PD
metadata snapshots are small, and a node mid-snapshot-load is not servable
anyway, so I left it. Worth remembering if PD snapshots ever grow.
--
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]
