bitflicker64 opened a new pull request, #3376:
URL: https://github.com/apache/hugegraph/pull/3376
## Purpose of the PR
- Step 1 of 3 toward closing #3235 (schema caches of Servers that share one
PD and HStore drift
apart). The full change, reviewed in ten rounds, is
hugegraph/hugegraph#322, which this PR and
the next two cut into pieces a PD reviewer and a Server reviewer can each
judge alone:
1. this PR: the PD side of one atomic schema commit (the `txn` RPC, the
per-graph record, the
replay guard, ReadIndex on reads);
2. PD delivers record changes over the existing watch with ACK and retry;
3. the Server commits schema through `txn`, syncs its caches from the
records and answers 503
until synced. Step 3 closes #3235 and supersedes the PD half of #3237.
- Implements the atomic-commit part of the maintainer's schema sync design:
a schema change and
its per-graph revision reach PD as one raft entry, so a crash between puts
cannot leave half a
change, and the revision is allocated at commit.
- Base is master, which holds #3368 (the ReadIndex this PR waits on) and
#3369.
- Org mirror: hugegraph/hugegraph#326 (same branch name, same body).
Today a schema DDL reaches PD as several separate puts, each its own raft
entry. A crash between
them leaves half a change, and nothing in PD records that the graph's schema
changed. This PR adds
the commit primitive; steps 2 and 3 build the notification and the Server
side on it.
### Why `txn` has no caller after this merges, and why that is fine
Nothing in PD or the Server calls `txn` until step 3 lands: the Server keeps
writing schema with
plain puts, and the apply listener has no subscriber until step 2. For every
existing deployment
this PR is inert. It lands first because the protocol, the applier and the
replay guard are the
part a PD reviewer can judge on their own, and the tests cover them without
a Server. The reads
do change: `get` and `scanPrefix` now wait for a ReadIndex (see below),
which #3368 made cheap.
The `txn` RPC and the read barrier are documented in
`hugegraph-pd/docs/api-reference.md` and
`configuration.md` in this PR.
### Scope: what this step holds, and what waits
The design doc names three responsibilities for the first release; this PR
is the PD half of the
first one and nothing else. Not here, by the doc's own scope:
- Conflict control for concurrent DDL from several Servers. One Server
writes a graph's schema at
a time; the doc keeps this out of the first release. The one compare the
Server will use is
"config absent" on create.
- Request deduplication or idempotency keys. PD keeps no request ids; a
`txn` is sent once and a
failure is an unknown outcome for the caller (see the client below).
- A graph lifecycle state machine (CREATING, COMMIT_CREATE, ADOPT, drop
markers). The record has
two states, LIVE and DROPPED, and an incarnation.
- Tombstone cleanup. A DROPPED record stays, one small key per dropped graph.
A review finding that needs one of these gets a reply pointing at this list
rather than a change.
Bugs in what is listed below get fixed here.
## Main Changes
One commit, 19 files, 878 production, 1,390 test and 91 doc lines.
**The `txn` RPC** on the KV service (`kv.proto`): compares (`ABSENT`,
`EQUALS`), ops (`PUT`,
`DELETE`, `DELETE_PREFIX`) and at most one graph record op. The response
carries `succeeded`,
`revision`, `incarnation`, `state`, `failure` and the failed compare. A
compare or an op with an
empty key is rejected before the TXN is proposed: to a `DELETE_PREFIX` an
empty key is the whole
KV space.
**The record** `HUGEGRAPH/{cluster}/SCHEMA_SYNC/{graphspace}/{graph}`, value
`{"rev": <raft index>, "inc": <incarnation>, "state": "LIVE" | "DROPPED"}`.
`rev` is the raft
index of the TXN that wrote it, so PD allocates it at commit, it only rises
per graph, and it
orders records across incarnations. The three record ops and what they
reject:
- `BUMP` sets `rev` to the raft index. With expected incarnation 0 it
creates an absent record as
LIVE incarnation 1 (a graph from before records, or one opened from
`conf/graphs`). It fails
with `GRAPH_DROPPED` on a dropped graph and `INCARNATION_MISMATCH` on
another incarnation.
- `CREATE` starts the next incarnation (or 1) as LIVE, whatever the stored
state, and marks the
record `"src":"create"`. The applier itself never refuses a CREATE; the
Server (step 3) guards
it with compares on its config key and on the record as it read it, so a
LIVE record is never
taken over.
- `DROP` marks the expected incarnation DROPPED. It fails with
`INCARNATION_MISMATCH` when the
record is absent or of another incarnation, and `GRAPH_DROPPED` when it is
dropped already.
So an instance of an earlier incarnation cannot write into a recreated
graph. A stored record
that is not a JSON object with positive integer `rev` and `inc` and a known
`state` fails every
TXN on it with `INVALID_RECORD` instead of counting as absent, which would
restart the
incarnation at 1. Only a missing key is absent: an empty value is a value.
**The `src` field** is an origin fact, not a lifecycle state. A CREATE
writes it, BUMP and DROP
keep it, nothing removes it, and a record a BUMP created has none. Step 3
uses it so that a REST
drop on a Server that does not serve the graph releases only a name an
unfinished create left
behind, never a `conf/graphs` graph another Server serves. The applier
parses it, and only the
value `create` counts as a mark.
**`KvTxnApplier`** runs the compares, checks the record, and writes the ops
plus the new record
in one RocksDB batch (`HgKVStoreImpl.writeBatch`). It reads only the store
and the entry. The
node that proposes a TXN builds that entry (`TxnEntry` in `kv.proto`,
internal to PD): the
request plus every key encoded as that node encodes the keys of `put`, so
every replica writes
the bytes the proposer chose whatever its own default charset, and `get`
reads what a TXN wrote.
A successful TXN with a record calls the registered `TxnApplyListener` on
the apply path; that
is the hook step 2 subscribes to, and a listener failure keeps the TXN.
**The replay guard.** jraft applies the entries after the last snapshot
again on restart, and
without a snapshot it does so over the store that already holds them. Plain
puts are blind
overwrites, so replaying them is harmless only in order. Each TXN writes its
own raft index under
one reserved key, `__TXN_LAST_INDEX` (`KvTxnApplier.LAST_INDEX_KEY`, not
under any prefix a
client reads), in the same batch, and on replay `RaftKVStore` skips every
store write, TXN or
plain, at or below that index. The one invariant: a store write whose raft
index is at or below
`__TXN_LAST_INDEX` is already in the store. Skipping only the TXNs would not
do: a plain put from
before the upgrade, replayed before a skipped TXN, would overwrite what the
TXN wrote. Plain
writes after the last TXN replay in order to the same store. TTL entries and
CLEAR are not
guarded, since they act on the in-memory TTL cache, not on RocksDB. A PD run
without raft
(`raft.enable=false`) has no log index and counts its TXNs under a key of
its own,
`__TXN_LOCAL_INDEX`, so a store switched to raft later does not take that
count for a raft
index. A live proposal whose index the store claims to hold already (a store
restored without
its log) is refused with an error to its caller instead of skipped: a
skipped write would
answer success and write nothing. Only a replay, which has no caller, is
skipped.
**Why a TXN is sent once.** PD keeps no request ids, and a TXN applied twice
is not always
harmless: a guarded CREATE fails its compare the second time. So
`KvClient.txn` makes one
attempt, plus one more on a new stub only when the connection was refused
(the TXN never left the
client). A follower forwards a TXN to the leader before proposing it; a
leader whose apply failed
answers with the error and never forwards, since the entry may be committed
already and the next
leader would apply it again. A throw is an unknown outcome, and the caller
reads the state before
retrying. A failed `txn` drops the client's stub only on UNAVAILABLE and
only if it still holds
the channel of the call, so it drops no stub another thread installed since;
the client throws a
`PDException` when a `txn` call gets no response, where the other KV calls
hit a
NullPointerException, and one that carries the gRPC status in its message
and cause when PD
answered with one, so a call an older PD rejected (UNIMPLEMENTED) is told
from a lost one
(UNAVAILABLE, DEADLINE_EXCEEDED).
**ReadIndex on `get` and `scanPrefix`.** A leader elected a moment ago may
not have applied its
whole log, so a plain read could return a record older than a committed TXN,
and a Server would
open a graph with a stale incarnation. Both reads wait for a raft ReadIndex
(the mechanism from
#3368) and fail with a `PDException` when the leader cannot confirm within
`raft.rpc-timeout`,
instead of returning an old value.
**Raft plumbing.** `KVOperation.TXN`, the closure adapter passes the apply
result to the
proposer (it dropped it before), and a TXN that fails in raft reaches the
caller with its cause.
### Decisions
Of the decisions recorded on hugegraph/hugegraph#322, the ones this step
carries:
1. Atomic commits and read barriers. One raft entry and one RocksDB batch;
revision = the raft
index; authoritative reads wait for a ReadIndex. The small generic
request shape (two compare
kinds, three op types, one record op) is kept rather than a purpose-built
schema-commit message:
the generality costs about 25 lines in the applier and 20 in the proto,
CREATE needs the
compare, and section 7 of the design leaves conditional commits open.
Alternative: a per-graph
counter in the record with no ordering across incarnations, or the
purpose-built message and a
second RPC later if compares are ever wanted.
2. No request ids and no dedup window (the PD half of decision 4). Send
once, unknown outcome on
failure, the caller reads the state before retrying. Alternative: a
request-id window in PD
(about 120 production lines and a cleanup task), which the earlier series
carried and the doc
keeps out of the first release.
3. Mixed-version writers (decision 11). An old Server's plain schema puts
are outside the
convergence guarantee, as section 7 of the doc says. The replay guard is
what keeps a PD restart
from reapplying such a put over a TXN; the old cache clear event stays
until every Server is
upgraded.
4. Landing shape (decision 8, updated). Three apache PRs, PD commit, PD
watch, Server, one at a
time, each cut from the reviewed branch of hugegraph/hugegraph#322.
Alternative: one PR, or two.
### Rollout
Upgrade every PD node before any Server of step 3. An old PD follower that
logs `Err op 10` after
the upgrade did not know the TXN entry and is rebuilt from a snapshot. This
PR alone changes no
Server behaviour, so Servers of the current release keep running against an
upgraded PD.
## Verifying these changes
- [x] Need tests and can be verified as follows:
- `KvTxnApplierTest` (17, `hg-pd-test`, in `PDCoreSuiteTest`):
`testOpsWrittenTogether` and
`testFailedCompareWritesNothing` (one batch or nothing),
`testEmptyValueIsNotAbsent`,
`testDeletePrefix`, `testRecordLifecycle`,
`testCreateMarksTheRecordForGood`,
`testDropChecksStateAndIncarnation`,
`testGuardedCreateWaitsForTheNameToBeDropped`,
`testIncarnationMismatch`, `testUnreadableRecordFailsTheTxn`
(INVALID_RECORD),
`testTxnSentAgainIsAppliedAgain` (why the client sends once),
`testListenerFailureKeepsTheTxn`, `testValidate` (empty keys and a
record op without its key
or expected incarnation are refused before the proposal),
`testKeysAreTheProposersBytes`
(an entry a proposer with another charset built is applied with its
bytes, and an entry
whose keys do not match its request is refused),
`testStoreWithoutRaftKeepsItsOwnCount`
(a store used without raft, then run with raft, skips nothing),
`testStoreAheadOfTheLogRefusesLiveWrites` (a live put or txn below the
stored index fails
with an error and writes nothing), and through a real raft group
`testRaftApplyReturnsTheResult`, `testRaftTxnFailureKeepsItsCause` and
`testRaftReplayOfLegacyPlainWritesAndTxns` (legacy plain writes and
TXNs replayed on a
restart without a snapshot keep the last writer).
- `KvServiceTest.testTxn` (`hg-pd-test`): the service path with a record.
- `KvServiceGrpcImplTest` (`hg-pd-service`):
`testGetAndScanReadAfterTheReadIndex`,
`testFailedReadIndexReturnsNoStoredValue` (an error and no stored
value, never an old one),
`testTxnIsRedirectedToTheLeader` (a follower forwards; a deposed
leader answers with the
error instead of forwarding).
- `KvClientTest` (`hg-pd-test`, against a packaged PD):
`testTxnLifecycleThroughRaft`,
`testConcurrentCreatesThroughRaft` (one winner),
`testTxnWithoutAResponseIsAPdException`,
`testOnlyAnUnavailableTxnDropsTheStub`,
`testRefusedTxnIsSentOnceMoreOnANewStub`,
`testTxnRefusedTwiceFails`, `testTxnRejectedByPdNamesTheStatus`
(UNIMPLEMENTED from a PD
without the RPC reaches the caller as that status, not as an
unreachable PD).
- Suites on this head (JDK 17, Linux, library, 2026-10-20,
run/failures/errors/skipped):
pd-common 83/0/0/0, pd-core 211/0/0/2, pd-client 90/0/0/0 against a
packaged PD started
with `travis/start-pd.sh`; the `hg-pd-service` module tests run inside
each of them;
`editorconfig:check` clean. Server unit and core suites are unchanged
by this PR; they ran on
the full branch of hugegraph/hugegraph#322 (unit 1384/0/0/8, core on
hstore 992/0/0/76).
- Replay negative control, on a packaged PD (library, 2026-10-18, script
`replay-neg.sh`
under the notes of hugegraph/hugegraph#322): with the guard narrowed
to skip only TXNs on
replay, a legacy put of `a=old`, a TXN put of `a=new` and a restart
without a snapshot leave
`a=old`; with the guard as in this PR the same sequence keeps `a=new`
(`testRaftReplayOfLegacyPlainWritesAndTxns` is the same check in a
unit test).
- The two-Server evidence (a change on one Server visible on the other
in about 20 ms, 503
within 2 s of PD stopping, service back 5 s after it returns) needs
all three steps and is
in step 3's description; the numbers are on hugegraph/hugegraph#322
today.
## Does this PR potentially affect the following parts?
- [ ] Dependencies
- [ ] Modify configurations
- [x] The public API: `txn` on the PD KV gRPC service, `KvClient.txn`, and
`get` and
`scanPrefix` wait for a ReadIndex (an error instead of a stale value
when the leader cannot
confirm within `raft.rpc-timeout`). `TxnEntry` in `kv.proto` is the
raft payload, internal
to PD.
- [x] Other affects: an old PD follower rebuilt from a snapshot after the
upgrade (Rollout above).
- [ ] Nope
## Documentation Status
- [x] `Doc - Done`: documentation is included here.
Documentation files in this PR or paired hugegraph-doc PR:
`hugegraph-pd/docs/api-reference.md` (the `Txn` RPC under the KV service,
the ReadIndex note
under `Get`, the "KV transactions" section of the Java client: the request
shape, the three
record ops and their failures, send-once and the unknown outcome, the replay
guard, and the
retry advice that now excludes `txn`) and
`hugegraph-pd/docs/configuration.md` (the
`raft.rpc-timeout` row names `get` and `scanPrefix` among the reads that
fail on the ReadIndex
deadline). The watch, the ACK and the record repair notes land with step 2.
--
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]
