Repository navigation
fix(pd): wait for a ReadIndex before serving get and scanPrefix - #3383
bitflicker64 wants to merge 1 commit into
Conversation
A leader elected a moment ago may not have applied every committed write yet, so a KV get or scanPrefix answered from its local store could return a value older than one a client already saw committed. Both reads now wait for a raft ReadIndex first, the barrier apache#3368 added for the id counters, and fail with an error and no value when the leader cannot confirm it within raft.rpc-timeout. The configuration doc says the deadline also bounds these two reads.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3383 +/- ##
============================================
- Coverage 55.93% 55.91% -0.02%
- Complexity 2277 2312 +35
============================================
Files 774 776 +2
Lines 68534 68865 +331
Branches 9333 9414 +81
============================================
+ Hits 38334 38509 +175
- Misses 26634 26770 +136
- Partials 3566 3586 +20
*This pull request uses carry forward flags. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
imbajin
left a comment
imbajin
left a comment
There was a problem hiding this comment.
Blocking: no. Summary: The ReadIndex barrier is ordered correctly, but its synchronous wait can saturate PD's shared gRPC executor under quorum loss. Evidence: exact-head static trace of GRpcServerConfig, PDConfig.ThreadPoolGrpc, and RaftEngine.waitReadIndex().
| * and a Server that read such a value would cache schema older than a revision it | ||
| * acknowledged: read after a ReadIndex barrier. | ||
| */ | ||
| RaftEngine.getInstance().waitReadIndex(); |
There was a problem hiding this comment.
waitReadIndex() blocks the calling gRPC worker for up to raft.rpc-timeout (10 seconds by default), and both new reads use the shared PD hg-grpc executor. With the default 600 core workers and an Integer.MAX_VALUE queue, 600 concurrent get/scanPrefix calls stalled by a quorum loss can occupy every worker while unrelated PD RPCs pile up. Please bound or isolate these waits so a burst of ReadIndex failures cannot starve the rest of the PD services. Evidence: GRpcServerConfig.configure installs HgExecutorUtil for the server, PDConfig.ThreadPoolGrpc defaults to 600 core workers and a queue of Integer.MAX_VALUE, and RaftEngine.waitReadIndex() waits synchronously for the configured timeout.
2 participants
Purpose of the PR
KvServiceGrpcImpl.getandscanPrefixanswer from the leader's local store. A leader elected amoment ago may not have applied every committed write yet, so a client can read a value older than
one it already saw committed: a Server that caches schema from a KV read would keep a stale copy
past a revision it acknowledged. #3368 put a ReadIndex barrier before PD reads an id counter; this
PR puts the same barrier, with the same deadline, before the two KV reads.
It was part of #3376 and was reviewed there; it does not depend on that PR and stands on its own.
Step 2 of the schema cache sync (the record watch with ACK and retry) needs both merged.
This PR is limited to the ReadIndex barrier before the two KV reads; a bug that already exists on
master in code this PR did not write is tracked as an issue, not fixed here.
Main Changes
One commit, three files, 8 production, 205 test and 1 doc lines.
KvServiceGrpcImpl.getandscanPrefixcallRaftEngine.waitReadIndex()before they read thestore. When the leader cannot confirm and apply a ReadIndex within
raft.rpc-timeout, the readfails with the error in the response header and no value, instead of a stored value that may be
stale. A follower redirects to the leader as before.
hugegraph-pd/docs/configuration.md: theraft.rpc-timeoutrow says the deadline also boundsthe KV
getandscanPrefix.Verifying these changes
KvServiceGrpcImplTest(new,hg-pd-service; it runs with every PD suite, since-ambuilds the module):
testGetAndScanReadAfterTheReadIndex(the ReadIndex runs before eachread, which then returns the stored value) and
testFailedReadIndexReturnsNoStoredValue(aReadIndex that times out, as on a leader without a quorum, is answered with an error and no
value, and the store is not read at all). The raft node is a proxy that answers each
ReadIndex as jraft does, by setting the result and running the closure.
editorconfig:checkclean, pd-common 83/0/0/0, pd-core 190/0/0/2, pd-client 83/0/0/0against a packaged PD (tests, failures, errors, skipped); the
hg-pd-servicemodule ran29 tests in each suite, master's 27 plus these two.
Does this PR potentially affect the following parts?
getorscanPrefixon a leader that cannot confirm aReadIndex within
raft.rpc-timeout(10 s by default) now fails instead of answering from itslocal store; a client retries as it does for any PD error. Each read waits one ReadIndex round
trip on a healthy leader.
Documentation Status
Select one option and provide the documentation location when applicable.
Doc - TODO: required documentation is pending; complete it before merging.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, theraft.rpc-timeoutrow.