bitflicker64 opened a new pull request, #3368:
URL: https://github.com/apache/hugegraph/pull/3368
## Purpose of the PR
- Related to #3235. A prerequisite of the schema cache sync design discussed
there.
The schema sync design asks for PD's consistent read, or the read barrier it
needs, before the notify and ACK protocol is built on it (section 5.1). PD has
no such barrier on master: `get` and `scanPrefix` read the local RocksDB with
only an `isLeader()` check, and id allocation reads its counter the same way.
This PR adds the barrier to id allocation and fixes two PD raft bugs found
while checking that path. It does not depend on the schema sync work and is
useful on its own.
## Main Changes
Three commits.
1. `fix(pd): wait for a raft ReadIndex before reading an id counter`
`IdMetaStore.getId()` reads the counter from local RocksDB, then writes
the next value through raft. Both `getCId()` overloads do the same with the
cyclic counter and its slots. A leader elected a moment ago could read the
counter before applying its predecessor's last write and hand out the same id
twice. `getId()` and `getCId()` now wait for a jraft ReadIndex first
(`RaftEngine.waitReadIndex()`, `HgKVStore.waitReadIndex()`).
EAGAIN (no entry of the new term committed yet) is retried every 20 ms
until the deadline, `raft.rpc-timeout`. EBUSY is jraft's answer during a
leadership transfer and also while its read index queue is full, so it is
retried with a delay that doubles from 20 ms up to 640 ms. Each ReadIndex
closure gets the time left to that deadline as its own timeout; created without
one, jraft fails it with ETIMEDOUT after its 2 s default
(`jraft.read-index.timeout`). A wait that times out or is interrupted stops its
pending retry, and every timeout reports the length of the wait. A node that is
shutting down fails the wait with NOT_LEADER instead of throwing jraft's
`IllegalStateException`. jraft also queues the closure with ENODESHUTDOWN or
EHOSTDOWN before it throws, so the closure maps those two codes to NOT_LEADER
as well, and the result is the same whichever completes first.
The wait alone does not protect the write that follows the read. A
request could read the counter in term 1 and pause, another node leads term 2
and allocates, the first node leads again in term 3, and the paused request
then writes its stale result and sets the counter back. The writes of `getId()`
and `getCId()` now carry the leader term taken before the wait
(`Task.setExpectedTerm`, through `RaftEngine.waitReadIndexInTerm()` and the
`HgKVStore` `put` and `remove` overloads that take a term). jraft rejects such
a write in any other term, and the allocation starts again from a fresh read,
three times at most.
2. `fix(pd): take the snapshot checkpoint on the state machine thread`
jraft calls `onSnapshotSave()` with the snapshot index set to the last
applied index, and applies the next entry as soon as it returns. PD handed the
whole save to a job thread, so the checkpoint could include entries past the
snapshot index, and a node installing it applied them twice. The checkpoint is
now taken before returning; only compression stays on the job thread. A failed
checkpoint completes the snapshot once with an error, where it used to report
the error and then success.
`onSnapshotLoad()` returned `true` when the archive could not be
decompressed or its checksum did not match, without loading anything, and jraft
then moved its applied index to the snapshot index. It returns `false` now, so
jraft fails the load. This also applies at startup: a PD whose local snapshot
is damaged now fails to start instead of starting with state it never loaded.
The snapshot error logs keep the stack trace.
3. `fix(pd): give the raft connect timeout its own option`
PD set jraft's connect, default and install-snapshot timeouts all from
`raft.rpc-timeout` (10 s). A candidate connects to each peer while it holds the
node lock, so a peer that accepts the TCP connection but never answers stalls
the election for the full connect timeout. Losing the first leader's host after
cluster start left PD without a leader for about 11 s. New
`raft.rpc-connect-timeout` (default 1000 ms, jraft's own default) and
`raft.rpc-install-snapshot-timeout` (default 0, which keeps today's value,
`raft.rpc-timeout`). The Helm chart docs said `pd.raftRpcTimeoutMs` bounds
leader elections. They now say that images with `raft.rpc-connect-timeout` use
that option for the Ping that opens a connection to a peer (set through
`pd.javaOpts`), that it does not bound requests on a connection that is already
open, and that `raft.rpc-timeout` is the timeout of that Ping on older images.
They and `configuration.md` also say that `raft.rpc-timeout` is the deadline of
the ReadIndex
wait in id allocation.
## Verifying these changes
- [ ] Trivial rework / code cleanup without any test coverage. (No Need)
- [ ] Already covered by existing tests, such as *(please modify tests
here)*.
- [x] Need tests and can be verified as follows:
- New tests: `RaftEngineReadIndexTest`, `IdMetaStoreReadIndexTest`,
`RaftStateMachineSnapshotTest`, `RaftEngineRpcTimeoutTest`, all in the PD core
suite, 37 tests in all.
- `RaftEngineReadIndexTest` covers a leader that answers EAGAIN 15 times
(about 300 ms) and still succeeds, a wait that hits its deadline with no retry
left running, a single ReadIndex that waits past jraft's 2 s default, a
shutting down node whose closure runs with ENODESHUTDOWN or EHOSTDOWN before it
throws, a node that answers EBUSY for 500 ms (ReadIndex calls at 0, 20, 60, 140
and 300 ms; 25 calls with a fixed 20 ms retry), the timeout message, and
`waitReadIndexInTerm()` on a leader, on a leader that is announced during the
wait, on a node whose term changes during the wait, and on a follower.
- `IdMetaStoreReadIndexTest` checks that `getId()` and both `getCId()`
overloads read only after the wait, write with the term of the wait, and leave
the store alone when the wait fails. Two tests run the term schedule above with
the production `IdMetaStore` and `RaftKVStore` over a raft engine that accepts
a task only in its expected term, as jraft's `NodeImpl.executeApplyingTasks`
does: node A reads in term 1 and pauses before its write, B allocates twice in
term 2, A leads again in term 3. `getId()` returns 20 and leaves the counter at
30; without the term on the write it returns 0 and sets the counter back from
20 to 10. `getCId()` returns 2; without the term it returns 0, which B handed
out.
- `RaftStateMachineSnapshotTest` saves a real archive and loads it three
ways: with its checksum (`true`, one load), with a wrong checksum and with the
archive missing (`false`, no load; both return `true` without the fix).
- Run on this head (2f8040974) on 2026-10-17, JDK 17.0.20, Linux x86_64:
- `mvn test -pl hugegraph-pd/hg-pd-test -am -P pd-core-test`: 187 run,
0 failures, 0 errors, 2 skipped.
- `-P pd-common-test`: 83 run, 0 failures, 0 errors.
- `mvn editorconfig:check`: clean.
- Run on the same three commits on the earlier base 000d538ab on
2026-10-11: `-P pd-client-test` against a PD started from the packaged dist (83
run, 0 failures, 0 errors), the CI package build, and `helm lint` with `helm
unittest helm/hugegraph` (199 passed). Each fix was also taken out in turn and
its test failed (7 runs).
## Does this PR potentially affect the following parts?
- [ ] Dependencies ([add/update
license](https://hugegraph.apache.org/docs/contribution-guidelines/contribute/#321-check-licenses)
info &
[regenerate_known_dependencies.sh](../install-dist/scripts/dependency/regenerate_known_dependencies.sh))
- [x] Modify configurations
- [ ] The public API
- [ ] Other affects (typed here)
- [ ] Nope
## Documentation Status
Select one option and provide the documentation location when applicable.
- [ ] `Doc - TODO`: required documentation is pending; complete it before
merging.
- [x] `Doc - Done`: documentation is included here or linked below.
- [ ] `Doc - No Need`: no user-visible documentation is affected.
Documentation files in this PR or paired hugegraph-doc PR:
- `hugegraph-pd/docs/configuration.md`, `application.yml` and
`application.yml.template` (the two new raft options)
- `helm/hugegraph/values.yaml` and `templates/_helpers.tpl` (what
`pd.raftRpcTimeoutMs` bounds now)
--
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]
