perf: Pause online delete if there any any gaps in recent ledger history - #5531
perf: Pause online delete if there any any gaps in recent ledger history#5531ximinez wants to merge 288 commits into
Conversation
b2c4c3d to
54f7f3c
Compare
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
I've been running this code locally off and on, and one thing I notice is I think this is ok, because that processing includes writing the ledger to disk, and is extremely important. So it makes sense to wait for it. I was concerned that this would cause rotation to stall indefinitely, and while it does pause a lot, it doesn't pause on every ledger, so it does make progress and eventually finish. I am, however, changing the default |
b1a5dc1 to
e13baa5
Compare
ximinez
added
the
JiraRefresh
bthomee
added
JiraRefresh
…elete-gaps * upstream/develop: chore: Remove codecov token check to support tokenless uploads on forks (5722) Set version to 2.6.0-rc3 Revert "perf: Move mutex to the partition level (5486)" chore: Update clang-format and prettier with pre-commit (5709) fix(test): handle null metadata for unvalidated tx in Env::meta (5715) chore: Workaround for CI build errors on arm64 (5717) chore: Fix file formatting (5718) fix: Skip notify-clio when running in a fork, reorder config fields (5712) chore: Reverts formatting changes to external files, adds formatting changes to proto files (5711)
113e704 to
cf83d92
Compare
|
Not sure how, but this branch got some changes from #5644 appended to it. I forced push to remove them and re-merge from |
![xrplf-ai-reviewer[bot]](https://avatars.githubusercontent.com/u/67929741?s=60&v=4){kind=small}
dangell7
left a comment
dangell7
left a comment
There was a problem hiding this comment.
Audited this and ran it A/B against develop on a lab network with a forced gap over the rotation trigger. It held rotation until the gap backfilled and rotated immediately after, gap-free rotations at parity with develop. Works as advertised.
Worth a release notes line that rotation now waits indefinitely on missing ledgers, so a node that stops rotating needs the gap filled, restarting won't clear it.
| std::this_thread::sleep_for(waitTime); | ||
|
|
||
| readServerStatus(index, age, mode, numMissing, lowerBound, unlock); | ||
| lastLedger = index; |
There was a problem hiding this comment.
lastLedger = index runs after the refresh, so index == lastLedger from the second iteration on and the trace branch never fires. Should be set before readServerStatus.
There was a problem hiding this comment.
Shoot. You're right. I should probably swap these two lines.
There was a problem hiding this comment.
Worth a release notes line that rotation now waits indefinitely on missing ledgers, so a node that stops rotating needs the gap filled, restarting won't clear it.
I'm addressing this here to keep the conversation together.
Unless I screwed up, "restarting won't clear it" is not true. The lower bound that is used to find gaps is lastGoodValidatedLedger_, which is set in run() after readyToRotate is decided. Now, healthWait() is the last factor in readyToRotate, but on the first iteration, lastGoodValidatedLedger_ == 0, and so the num missing check returns 0. From that point on, lGVL_ is updated regardless of the value of readyToRotate, so it keeps up more-or-less with the current validated ledger.
tl;dr A restart will allow a rotation to trigger without fully back filling. The design and intention of the gap detection is only to stop for gaps that happen during rotation, presumably because they were caused by rotation. Fortunately, currently on well-behaved nodes, gaps only start showing up when rotation is running (or a network outage or whatever).
Example: Your node starts with deleteInterval_ = 256 and lastRotated == 1300 and has the range 1000-1300,1350-1600 available. Once the node is synced, the range is say 1000-1300,1350-1600,1695-1700. At that point, readyToRotate will be true because validatedSeq (1700) >= lastRotated(1300) + deleteInterval_(256), canDelete_ is not relevant, and the healthWait() passed because lastGoodValidatedLedger is either 0 or 1699. Either way, there are no ledgers detected missing.
There was a problem hiding this comment.
This is a well-scoped, carefully implemented change. The new missingFromCompleteLedgerRange() check is correctly locked (using the now-mutable completeLock_), the healthWait() refactor properly releases mutex_ before touching ledgerMaster_'s completeLock_ (avoiding the deadlock the comments call out), and the rendezvous() timeout overload preserves the same default behavior for existing callers. Test coverage (LedgerMaster_test, SHAMapStore_test) closely mirrors the new production logic. I did not find any changed lines with a clear correctness, security, or resource-leak bug; the one item below is a minor log-string typo.
There was a problem hiding this comment.
This MR adds a gap-detection check (missingFromCompleteLedgerRange) to online delete's healthWait() loop so rotation pauses if recent ledger history has holes, and lowers the recovery poll interval from 5s to 2s. The core logic (RangeSet diff, lock ordering via ScopeUnlock to avoid mutex_/completeLock_ deadlock, new rendezvous(timeout) API) looks carefully done and well covered by new SHAMapStore_test/LedgerMaster_test cases. One area worth a second look: healthWait() feeds lastGoodValidatedLedger_ (set from validatedSeq in run()) and a freshly-read getValidLedgerIndex() into missingFromCompleteLedgerRange, whose first>last branch is guarded only by an UNREACHABLE/abort — worth confirming these two sequence sources can never diverge such that index < lastGoodValidatedLedger_.
High Level Overview of Change
Adds another check to the online deletion process to prevent ledger gaps from persisting if the node goes out of sync.
Context of Change
Some nodes struggle to stay in sync when online delete is running due to the added I/O burden. This has proven difficult to avoid without upgrading hardware. If the process takes a particularly long time, which it can with large datasets, the node can end up with many gaps in the recent ledger history. Those gaps will not be filled until the node is idle enough for low-priority historical ledger requests to run.
Note that this is a trade-off. The online deletion may take longer to complete, but the node will stay more up-to-date.
Type of Change