Skip to content

feat(pd): commit schema keys and a per-graph record as one raft entry - #3376

Open
bitflicker64 wants to merge 1 commit into
apache:masterfrom
hugegraph:task/schema-sync-pd-txn
Open

bitflicker64 wants to merge 1 commit into
apache:masterfrom
hugegraph:task/schema-sync-pd-txn

Conversation

@bitflicker64

@bitflicker64 bitflicker64 commented Oct 20, 2026 •

Copy link
Copy Markdown
Contributor

Purpose of the PR

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 txn
RPC is documented in hugegraph-pd/docs/api-reference.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.

This PR is step 1 of the schema cache sync feature and is limited to that step. A defect in lines
this PR writes is fixed here. A bug that already exists on master in code this PR did not write,
even one this change makes visible, is not fixed here: it gets an issue that tracks it, linked
from the thread. Anything beyond the design's scope list gets a reply citing the list, not a
change. The TTL cleanup that deletes a marker-shaped name instead of the expired key is such a
bug: #3378, fixed in #3382; this PR keeps only its own part, a TXN deleting the marker of a
value it replaces.

Main Changes

One commit, 28 files, 1,628 production, 1,958 test and 128 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. Each enum starts with
UNSPECIFIED = 0, so a field a caller never set is not a real operation. A compare or an op with
an empty key, or one left at UNSPECIFIED, is rejected before the TXN is proposed, with the new
error type TXN_NOT_PROPOSED (nothing was applied, unlike UNKNOWN, the unknown outcome): 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, or the previous rev plus one when that is higher (a store that
changed raft.enable starts its index low again), so PD allocates it at commit, it only rises
per graph, and it orders records across incarnations. The response's revision is that rev.
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. A value
that was put with a TTL has a cleanup marker next to it, and the TTL cleanup is meant to delete
the key a marker names once its time has passed, whatever the key holds by then (it deletes
another name today, #3378); so an op that replaces or deletes such a value deletes its marker in
the same batch (KvService.ttlMarkerKey: the marker's ASCII prefix is encoded in the
proposer's charset, which the entry carries, and the key's own stored bytes follow it unchanged,
so every node names the marker getTTLStoreKey wrote, whatever its own default charset, and a
key an older node wrote in another ASCII-compatible charset keeps its marker's name). 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. Only a replay is skipped: RaftEngine records the log's last index when the node starts,
and an entry at or below it is one the node applied in an earlier run. An entry above it that
the store already claims means the store is ahead of its log (restored from a backup without the
log); the other nodes apply it, and this node fails the apply instead of skipping it, on every
node and whether the entry has a caller or not: RaftStateMachine.onApply turns the failure
into setErrorAndRollback, jraft stops the state machine, a leader steps down, the probes from
#3368 and #3358 report the error, and the node logs the entry, the stored index and the recovery
(remove its data directory, rejoin from a snapshot) at error level. The same fail-stop covers a
store whose TXN index is beyond the end of the log at start, and a replay from another raft log
than the store was written from (a log rebuilt or replaced while the store was kept): the index
is stored with its term, and a replay is skipped only while the log has that term at that index
(a log that does not hold the index fails too; only an index stored without its term, or a log
that cannot be read at all, replays unchecked, logged once).
By raft's log matching property one index is enough: equal terms there mean the same entries
below it.

The peer gate. A PD from before this PR logs an unknown op and goes on, so during a rolling
upgrade an updated leader could commit TXN entries an old peer never applies, and that peer
would serve stale metadata once it leads. The leader proposes a TXN only once every other voter
and learner of the configuration has answered its member probe (GetMemberResponse, new field
maxKvOp; a release before it has no such field, so it reads as 0) as a release that applies the
entry. TxnPeerGate keeps that answer, and the leader records the confirmed endpoints, itself
included, through raft under a raw key (__TXN_CONFIRMED_PEERS, written without waiting), so a
new leader after a failover trusts what an earlier leader confirmed and probes only the rest (a
release cannot go down; a PD downgrade after the first TXN is unsupported). A peer that never
answered, or answered as an older release, blocks every TXN with TXN_NOT_PROPOSED naming it; a
confirmed peer that is unreachable later does not block, on this leader or the next.
Unconfirmed peers are probed again on a TXN attempt at most every 3 s and at once when the
configuration changes, so a node added to the group blocks until it answers. A peer that joins
catches up by applying the log, TXN entries included, so both membership paths
(RaftEngine.changePeerList, PDService.updatePdRaft) probe every peer the change adds first and
refuse an unconfirmed or older one with TXN_NOT_PROPOSED; that also closes the window between a
TXN's check and its proposal. Peers the change keeps are not probed: the check before every TXN
covers them, so a kept member that is down or not upgraded yet does not stop a membership change.

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).

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.
TxnApplyListener, the hook step 2 subscribes to, states its repeats: after a snapshot at S the
TXNs past S are applied again and it is called again with the same index and revision; without a
snapshot they are skipped; on the path without raft it runs on the gRPC thread under the store's
write lock.

Decisions

Of the decisions recorded on hugegraph#322, the ones this step carries:

  1. Atomic commits. One raft entry and one RocksDB batch; revision = the raft index (the read
    barrier on get and scanPrefix is fix(pd): wait for a ReadIndex before serving get and scanPrefix #3383). 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 feat(pd,server): converge schema caches across HStore Servers through PD hugegraph/hugegraph#322. Alternative: one PR, or two.

Rollout

Upgrade every PD node before any Server of step 3, and before the first txn. The peer gate
enforces the order: a leader proposes no TXN until every other voter and learner has answered
its probe as an upgraded release, and names the peer that blocks it. A peer that is down during
the upgrade stays unconfirmed, so it blocks txn until it is back and answers, which is the
point: an old peer that replays the log on its return would skip the entries. A PD downgrade
after the first TXN is unsupported, and an old-release PD can no longer be added to an upgraded
group: the membership change is refused and names it. A membership change probes only every
peer it adds, so a kept member that is down or not upgraded yet does not stop it. This PR alone
changes no Server behaviour,
so Servers of the current release keep running against an upgraded PD.

Verifying these changes

  • Need tests and can be verified as follows:
    • KvTxnApplierTest (22, 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),
      testStoreAheadOfTheLogFailsTheApply (a newer entry the store claims, or a store index
      beyond the end of the log, fails the apply on a follower and through the proposer path, and
      writes nothing), testReplayChecksTheLogGeneration (a replay is skipped while the log has
      the stored term at the stored index, fails on another term or a log without the index, and
      an index stored without its term, or an unreadable log, replays as before),
      testTxnDropsStaleTtlMarkers (markers seeded as getTTLStoreKey writes them),
      testTtlMarkerIsNamedInTheProposersCharset (a UTF-16 proposer),
      testTtlMarkerOfAKeyWrittenInAnotherCharsetIsDropped (an ISO-8859-1 key and marker found by
      a UTF-8 prefix delete), testUnsetEnumFieldsAreRejected, and against
      RaftKVStore with a mocked RaftEngine (invoke called as the state machine calls it,
      or addTask answered as raft would) testRaftApplyReturnsTheResult (the result reaches
      the proposer's closure), testRaftTxnFailureKeepsItsCause (a deposed leader's failure
      keeps its cause) and testRaftReplayOfLegacyPlainWritesAndTxns (legacy plain writes and
      TXNs replayed on a restart without a snapshot keep the last writer).
    • TxnPeerGateTest (7, hg-pd-test, in PDCoreSuiteTest): an old peer blocks and names
      itself, new peers allow, a confirmed peer that goes unreachable does not block, a peer
      added to the configuration blocks until it answers, a confirmed peer is not probed again,
      a membership change probes the peers it adds at once and is refused by an older or silent
      one, and testConfirmationsSurviveALeaderChange: after a failover the new leader, with one
      confirmed peer down, still proposes, while a peer no leader confirmed blocks;
      KvTxnApplierTest.testConfirmedPeersAreRecordedThroughRaft shows the record going through
      raft as one PUT and reading back; RaftEngineIpAuthIntegrationTest.testChangePeerListRefusesAPeerWithoutTheTxnEntry
      shows changePeerList stopping before changePeers for such a peer, and
      testChangePeerListIgnoresKeptMembers that an older or silent kept member does not.
    • KvServiceTest.testTxn (hg-pd-test): the service path with a record.
    • KvServiceGrpcImplTest (hg-pd-service): testTxnIsRedirectedToTheLeader (a follower
      forwards; a deposed leader answers with the error instead of forwarding).
    • RaftEngineReadinessTest (hg-pd-test): testShutdownDropsTheTxnPeerGate (a txn after
      shutdown fails as any proposal without a node) and
      testPeerChecksOffTheLeaderAnswerWithAPdException (a node that lost the leadership answers a
      txn with TXN_NOT_PROPOSED and a membership change with NOT_LEADER, as jraft lists the
      configuration only on the leader).
    • 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-21, run/failures/errors/skipped):
      pd-common 83/0/0/0, pd-core 229/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 feat(pd,server): converge schema caches across HStore Servers through PD 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 feat(pd,server): converge schema caches across HStore Servers through PD 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 feat(pd,server): converge schema caches across HStore Servers through PD hugegraph/hugegraph#322 today.

Does this PR potentially affect the following parts?

  • Dependencies
  • Modify configurations
  • The public API: txn on the PD KV gRPC service, KvClient.txn, and the error type
    TXN_NOT_PROPOSED in pdpb.proto. TxnEntry in kv.proto is the raft payload, and maxKvOp on the member probe's response
    the capability a peer reports; both internal to PD.
  • Other affects: the PD rollout order and the peer gate (Rollout above).
  • Nope

Documentation Status

  • 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, and 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, the peer gate, and the retry
advice that now excludes txn). The watch, the ACK and the record repair notes land with step 2;
the read barrier and its configuration.md row land with #3383.

@codecov

codecov Bot commented Oct 20, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 87.09677% with 36 lines in your changes missing coverage. Please review.
✅ Project coverage is 56.11%. Comparing base (c2ec570) to head (9c5e6ef).
⚠️ Report is 6 commits behind head on master.

Files with missing lines Patch % Lines
...va/org/apache/hugegraph/pd/store/KvTxnApplier.java 89.44% 9 Missing and 12 partials ⚠️
...org/apache/hugegraph/pd/client/AbstractClient.java 84.61% 3 Missing and 3 partials ⚠️
...c/main/java/org/apache/hugegraph/pd/KvService.java 71.42% 1 Missing and 3 partials ⚠️
.../java/org/apache/hugegraph/pd/client/KvClient.java 50.00% 2 Missing and 1 partial ⚠️
...a/org/apache/hugegraph/pd/store/HgKVStoreImpl.java 90.47% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master    #3376      +/-   ##
============================================
+ Coverage     55.93%   56.11%   +0.17%     
- Complexity     2277     2437     +160     
============================================
  Files           774      777       +3     
  Lines         68534    69144     +610     
  Branches       9333     9484     +151     
============================================
+ Hits          38334    38797     +463     
- Misses        26634    26741     +107     
- Partials       3566     3606      +40     
Flag Coverage Δ *Carryforward flag
commons 96.37% <ø> (ø) Carriedforward from c2ec570
hstore 35.98% <ø> (+0.17%) ⬆️
pd 51.45% <87.09%> (+2.27%) ⬆️
server_hbase 41.84% <ø> (-0.06%) ⬇️ Carriedforward from c2ec570
server_memory 54.29% <ø> (-0.04%) ⬇️ Carriedforward from c2ec570
server_rocksdb 42.29% <ø> (-0.06%) ⬇️ Carriedforward from c2ec570
store 31.88% <0.00%> (-0.06%) ⬇️

*This pull request uses carry forward flags. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@imbajin imbajin left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking: yes. Summary: Three data-integrity issues remain in the transaction apply path: a local store/log mismatch can skip a committed entry, older peers can advance past the new opcode without applying it, and a stale TTL marker can delete a newer transaction value. Evidence: verified against exact-head Raft apply, opcode handling, and TTL cleanup paths at 7c89d8b; the latest-head Actions run passed; no local tests were run.

@bitflicker64
bitflicker64 force-pushed the task/schema-sync-pd-txn branch from 7c89d8b to 8f29ffd Compare October 20, 2026 14:38
@bitflicker64
bitflicker64 requested a review from imbajin October 20, 2026 14:41
@bitflicker64
bitflicker64 force-pushed the task/schema-sync-pd-txn branch 5 times, most recently from e2ba916 to 7c8121d Compare October 20, 2026 16:58

@bitflicker64 bitflicker64 left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking: yes. Summary: The txn applier, replay guard, store-ahead fail-stop, peer gate on txn and client send-once path hold up at this head, and the three earlier threads are addressed. One Important finding remains: the new membership-change gate also probes peers that are already in the group, so changePeerList and updatePdRaft are refused when a kept peer is down or not yet upgraded. That changes behaviour on deployments that never call txn, while the description says this PR is inert for them. Evidence: read the full diff of 7c8121d against merge base 86c05ef and traced KvTxnApplier (compares, record check, DELETE_PREFIX batch pruning, TTL-marker drop, which matches getTTLStoreKey), RaftKVStore.appliedBefore/invoke against RaftStateMachine.onApply (a throw reaches setErrorAndRollback), onSnapshotSave (synchronous checkpoint) and HgKVStoreImpl.loadSnapshot (replaces the whole DB, index key included), RaftEngine.init (lastLogIndexAtStart read after start; the MAX_VALUE default only widens the skip), TxnPeerGate, changePeerList, PDService.updatePdRaft, KvServiceGrpcImpl.txn and redirectToLeader, and AbstractClient.blockingUnaryCallOnce. The earlier TTL-cleanup interleaving thread no longer applies: this head does not change clearTTLData, and the master cleanup does not delete the value key (tracked in #3378). The new tests are registered in PDCoreSuiteTest and PDClientSuiteTest, and the hg-pd-service tests run through -am. CI: pd_store / pd passes. pd_store / store fails in ScanLifecycleTest.testReceivePreservesExternalInterrupt, in Store code this PR does not touch. riscv64 fails on a Maven Central connection reset. No tests were run for this review.

@bitflicker64 bitflicker64 left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking: yes. Score: 6/10. Summary: The txn applier, the replay guard (with the new term check and the stop when the stored index is past the end of the log), the TTL marker drop, the client's send-once path and the gate on added peers hold up at this head, and the earlier threads are answered. One Important finding: the peer gate's confirmations do not survive a leader change, so a failover with one PD node down blocks every txn while the group keeps a majority. Evidence: read the full diff of e847cfa against merge base 86c05ef, through the apply path (RaftStateMachine.onApply, RaftKVStore, KvTxnApplier, HgKVStoreImpl), the gate and both membership paths, and the gRPC and client txn paths. Ran KvTxnApplierTest (23), RaftEngineReadinessTest (15) and TxnPeerGateTest (7, with one failover case added locally) on this head (JDK 17, Linux, 2026-10-21): 0 failures. CI: struct, rocksdb-compatibility and check-license pass; pd_store / pd, store and hstore, cluster-test, docker-build and riscv64 are still pending.

@bitflicker64 bitflicker64 left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking: no. Summary: The txn applier, the replay guard (skip only at or below the log end read at start, term check at the stored index, fail-stop when the store is ahead), the TTL marker drop, the peer gate with confirmations recorded through raft, the probe of added peers on both membership paths, the follower forward and the client's send-once path hold up at this head, and every earlier thread is answered. One Minor cleanup: a reflection helper this PR adds has no caller. Evidence: read the full diff at 360752a against merge base 86c05ef with RaftStateMachine.onApply/onSnapshotSave/onSnapshotLoad, HgKVStoreImpl.saveSnapshot/loadSnapshot (the checkpoint is taken on the FSM thread and a load replaces the DB, so after a snapshot every replayed entry is above the stored TXN index), MetadataFactory (one RaftKVStore, so one task handler per entry), KVOperation serialization (index and term are not written), KvService key and TTL marker encoding, ServiceGrpc.redirectToLeader and AbstractClient.resetStub/getBlockingStub; checked that the new test classes are in PDCoreSuiteTest/PDClientSuiteTest; CI pd_store / pd, cluster-test and codecov are green on this head.

@imbajin imbajin left a comment •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update, Oct 21 05:37 UTC: the conditional historical-prefix marker finding below is fixed in the current revision. Two independent bounded checks using the updated extracted Java helper preserve the original historical e9 suffix; the 35 prior same-charset/fallback/error cases and the reported regression pass. Static inspection also confirms that the new regression test constructs its historical marker independently of the helper under test. The earlier finding below is retained as history, not an outstanding issue on this revision. This verifies the specific byte-preservation repair only: we did not run the repository JUnit suite, RocksDB/Raft integration, failover or performance acceptance, and do not assign a whole-PR score or inherit prior-head CI.


[P2, conditional compatibility] Preserve historical key bytes when reconstructing TTL markers

At 1d9dae54f83137a44e56d2f3dd164e412c75309d, ttlMarkerKey decodes every scanned key using the current TXN proposer's charset. That is not necessarily the charset of a historical writer. For example, an old ISO-8859-1 K@t/é key matches a UTF-8 DELETE_PREFIX("t/"), but the helper replaces its e9 suffix with efbfbd, so the batch targets a different marker and leaves the original behind. The previous byte-copy helper preserved that suffix.

Please preserve historical suffix bytes where appropriate and add a cross-charset historical-prefix test, or explicitly document and validate a stable-charset storage prerequisite. I did not find such a restriction in the current PR description. This is conditional on historical mixed/default-charset changes, not a failure of normal uniform UTF-8 operation.

Evidence: two reviewers independently ran the extracted Java helper: 35 same-charset/fallback/error cases passed, and the one historical ISO-8859-1/UTF-8 prefix counterexample reproduced. This is helper/prefix evidence, not RocksDB, cleanup, Raft-cluster, performance or data-loss verification. No overall score is assigned.

Sources: marker reconstruction, historical scanned keys.

A schema DDL reaches PD as several separate puts, each its own raft
entry, so a crash between them leaves half a change and no signal that
anything changed. Add a txn rpc that applies compares (ABSENT, EQUALS),
PUT, DELETE and DELETE_PREFIX ops and an optional per-graph record
change as one raft entry and one RocksDB write batch. 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, and so is a compare,
op or record whose enum field is left at its zero value, UNSPECIFIED.
Both answer with the new error type TXN_NOT_PROPOSED, so a caller can
tell a request PD never proposed from an unknown outcome. The node that
proposes a TXN encodes its keys as it encodes the keys of put and ships
them in the entry with its charset name, so every node writes the same
bytes whatever its own default charset. A TXN that replaces or deletes a
value put with a TTL deletes the TTL marker next to it in the same
batch: the TTL cleanup is meant to delete the key a marker names once
its time has passed, whatever the key holds by then (it deletes another
name today, apache#3378). The marker's ASCII prefix is encoded
in the proposer's charset and the stored key's bytes follow it as they
are, so a key an older node wrote in another charset keeps its marker.

The record, HUGEGRAPH/{cluster}/SCHEMA_SYNC/{graphspace}/{graph}, holds
{"rev","inc","state"}: rev is the raft index of the TXN that wrote it,
or the previous rev plus one when that is higher (a store that changed
raft.enable starts its index low again), so PD allocates it at commit
and it only rises per graph; inc is the graph's incarnation; state is
LIVE or DROPPED. BUMP advances rev and
creates the record (LIVE, incarnation 1) the first time a graph from
before records is written. CREATE starts the next incarnation, LIVE, and
marks the record "src":"create", which BUMP and DROP keep: the origin of
a record, not a state. DROP marks the expected incarnation DROPPED. A
BUMP or DROP of a DROPPED graph or of another incarnation is rejected,
so an instance of an earlier incarnation cannot write into a recreated
graph. A stored record that is not a JSON object with positive integers
rev and inc and a known state fails the TXN with INVALID_RECORD instead
of counting as absent, which would restart the incarnation at 1.

The apply reads only the store and the request. jraft applies the log
after the last snapshot again on restart, over the store that already
holds it, so each TXN writes its index and every store write (TXN or
plain put/delete) at or below the last TXN index is skipped on replay: a
replayed plain put from before the upgrade would otherwise overwrite
what a skipped TXN wrote. Plain writes after the last TXN replay in
order to the same store. A TXN is not deduplicated, so it is sent once:
KvClient.txn makes one attempt, plus one more on a new stub only when
the connection was refused, so the TXN never left the client, and a
leader whose apply failed answers with the error instead of forwarding
the TXN, since the entry may be committed already and the next leader
would apply it again. A failure is an unknown outcome for the caller.
A store without raft counts its TXNs under a key of its own, so a store
switched to raft later does not take that count for a raft index. The
raft path stores the term with the index, and a replay is skipped only
while the log has that term at that index and the stored index is
within the log; otherwise the apply fails as below. Only
a replay is skipped, an entry that was in the node's log when it
started; an entry above that index that the store claims to hold means
the store is ahead of its log (restored from a backup without the log),
and the apply fails on that node instead of skipping it, since the
peers apply the entry: jraft stops the state machine, a leader steps
down, the probes report the error, and the node rejoins from a snapshot
after its data directory is removed.

A PD from before the TXN entry logs an unknown op and goes on, so a
rolling upgrade could commit entries an old peer never applies. The
leader proposes a TXN only once every other voter and learner of the
configuration has answered its member probe as a release that applies
the entry (a new field of the probe's response); the leader records
the confirmed endpoints, itself included, through raft, so a new leader
after a failover trusts them, an unconfirmed peer blocks the TXN with
TXN_NOT_PROPOSED naming it, a confirmed peer that is unreachable later
does not, and unconfirmed peers are probed again at most every 3 s and
at once when the configuration changes. A peer that joins catches up by
applying the log, TXN entries included, so both membership paths probe
every peer the change adds before changing it and refuse an unconfirmed
or older one; that also closes the window between a TXN's check and its
proposal. Kept peers are covered by the check before every TXN. A node
that lost the leadership before it could list its peers answers with
TXN_NOT_PROPOSED.

A successful TXN with a record calls the registered TxnApplyListener
on the apply path, the hook of the schema sync dispatch. The closure
adapter passes the apply result to the proposer, which it dropped
before, and a TXN that fails in raft reaches the caller with its
cause. 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 when PD answered with one, so a call an
older PD rejected (UNIMPLEMENTED) is told from a lost one. 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.

hugegraph-pd/docs/api-reference.md documents the txn RPC, the send-once
contract, the replay guard and the peer gate.
@bitflicker64
bitflicker64 force-pushed the task/schema-sync-pd-txn branch from 1d9dae5 to 9c5e6ef Compare October 21, 2026 05:03
@bitflicker64

Copy link
Copy Markdown
Contributor Author

@imbajin Fixed in 9c5e6ef. ttlMarkerKey no longer decodes the key. It checks the K@ prefix in the proposer's charset, keeps the stored key's remaining bytes as they are, and puts the encoded K@T@<time>@ prefix before them. Your ISO-8859-1 K@t/é key now maps to its own marker, suffix e9, under a UTF-8 DELETE_PREFIX("t/"). The UTF-16 proposer case still names the marker getTTLStoreKey writes.

testTtlMarkerOfAKeyWrittenInAnotherCharsetIsDropped seeds that historical key and marker, applies the UTF-8 prefix delete, and checks both are gone. Suites on the new head (JDK 17, Linux, 2026-10-21): pd-common 83/0/0/0, pd-core 229/0/0/2, pd-client 90/0/0/0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] Schema changes on one HStore Server can stay invisible on the others indefinitely

2 participants