Repository navigation
fix(pd): keep RaftEngineReadIndexTest stubs clear of the alive-peers refresher - #3385
Mihir-Rabari wants to merge 2 commits into
Conversation
RaftEngineReadIndexTest swaps a mock into the RaftEngine singleton and re-stubs readIndex on it inside its tests. When an earlier suite test leaves the singleton started and leading, the pd-raft-alive-peers refresher calls listAlivePeers() on that same mock whenever the state machine reports a leader, and a call from that thread while a doAnswer(...).when(mock).readIndex(...) is in progress is what Mockito reports as an UnfinishedStubbingException, so the suite fails now and then. Announce no leader term in setUp, before the mock is installed, so the refresher leaves the mock alone, and announce a term in testReadIndexInTermKeepsTheTermTakenBeforeTheWait only after its re-stubbing has completed. The refresher itself is untouched: it still reads the node and the term exactly as before whenever the singleton leads. Fixes apache#3384
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3385 +/- ##
============================================
+ Coverage 55.93% 55.94% +0.01%
- Complexity 2277 2309 +32
============================================
Files 774 776 +2
Lines 68534 68865 +331
Branches 9333 9414 +81
============================================
+ Hits 38334 38527 +193
- Misses 26634 26746 +112
- Partials 3566 3592 +26
*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:
|
bitflicker64
left a comment
bitflicker64
left a comment
There was a problem hiding this comment.
Blocking: yes. Summary: The change keeps the alive-peers refresher away from the mock, but other threads still call it, so #3384 can still happen: the TaskScheduleService executors that PDCoreTestBase leaves running call isLeader(true) on the installed mock while leaderTerm is -1. Evidence: RaftEngine.isLeader(), TaskScheduleService.init() and PDCoreTestBase.shutdownService() at af1b483. I also ran mvn test -pl hugegraph-pd/hg-pd-test -P pd-core-test on this head with a probe added just before this class. The mock got 42 isLeader(true) calls from 8 pools in 5 s. A loop of the answerShutdown stubbing against a thread calling RaftEngine.getInstance().isLeader() failed with UnfinishedStubbingException after 13 re-stubs, and the same loop without that thread stayed clean for 2,000,000.
| // unfinished stubbing. Keep the machine not-leader while the mock is | ||
| // installed: the refresher then leaves the mock alone, and the tests below | ||
| // announce a term only once every stubbing has completed. | ||
| this.leaderTerm.set(-1L); |
There was a problem hiding this comment.
Important: leaderTerm at -1 keeps the refresher off this mock, but other threads still call it. RaftEngine.isLeader() calls raftNode.isLeader(true) without reading leaderTerm. PDCoreTestBase.initService() starts a TaskScheduleService for each of the 8 suite classes before this one, and shutdownService() never stops it, so their 1 s clearTTLData task (if (isLeader())) keeps calling isLeader(true) on this mock. A call from another thread between doAnswer(...).when(this.mockNode) and .readIndex(...) takes the pending answer, which gives the #3384 UnfinishedStubbingException. A probe placed just before this class in PDCoreSuiteTest saw 42 such calls in 5 s from 8 executor pools.
Please stub readIndex only once, in setUp before the mock is installed, with an answer that delegates to a volatile Answer<Void> field. The tests that now call doAnswer/doThrow(...).when(this.mockNode) would assign that field instead. The order of the leaderTerm writes and the setUp comment then no longer matter.
PDCoreTestBase.shutdownService() tears down the raft engine but not the TaskScheduleService it started, so every core suite class leaves a pool behind that keeps polling RaftEngine.isLeader() every second. The polls land on the node mock RaftEngineReadIndexTest installs, and one arriving from that thread while a test re-stubs the mock is again an UnfinishedStubbingException, so guarding the refresher alone does not close the flake. Stop the pollers before the engine, the same order the server's shutdown hook closes them in, and add a test to RaftEngineReadIndexTest that fails while any such poller is left running.
2 participants
Fixes #3384
RaftEngineReadIndexTest swaps a mock into the RaftEngine singleton and re-stubs
readIndex on it inside its tests. Two background callers can reach that mock
from other threads, and a call arriving while a
doAnswer(...).when(mock).readIndex(...) is in progress is what Mockito reports
as an UnfinishedStubbingException:
singleton's node whenever the state machine reports a leader;
shutdownService() tore down the raft engine only, so each of the eight core
suite classes that run before RaftEngineReadIndexTest leaked a pool that
keeps polling RaftEngine.isLeader() every second.
The first commit keeps the refresher away from the mock: setUp announces no
leader term before installing the mock, and the test that needs a term
announces it only after its re-stubbing has completed. The refresher itself is
untouched — it still reads the node and the term exactly as before whenever
the singleton leads.
The second commit closes the poller leak pointed out in review:
PDCoreTestBase.shutdownService() now stops the TaskScheduleService it started,
before the engine it polls, the same order ShutdownHook.closeOwners() uses,
and RaftEngineReadIndexTest gains a test that fails while any leaked poller
reaches the node mock. On the first commit alone that test goes red within
0.2 s (NeverWantedButInvoked: node.isLeader(true), from RaftEngine.isLeader);
with the leak closed the suite runs clean.
Verification: mvn test -pl hugegraph-pd/hg-pd-test -P pd-core-test twice on
the branch head (Tests run: 191, Failures: 0, Errors: 0 both runs),
editorconfig:check and the apache-rat license check clean.