Skip to content

perf: Pause online delete if there any any gaps in recent ledger history#5531

Open
ximinez wants to merge 275 commits into
developfrom
ximinez/online-delete-gaps
Open

perf: Pause online delete if there any any gaps in recent ledger history#5531
ximinez wants to merge 275 commits into
developfrom
ximinez/online-delete-gaps

Conversation

@ximinez

@ximinez ximinez commented Jul 2, 2025

Copy link
Copy Markdown
Contributor

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

  • New feature (non-breaking change which adds functionality)

@ximinez
ximinez requested a review from a team July 2, 2025 21:34
@ximinez
ximinez force-pushed the ximinez/online-delete-gaps branch 5 times, most recently from b2c4c3d to 54f7f3c Compare July 2, 2025 22:47
@codecov

codecov Bot commented Jul 2, 2025

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 92.85714% with 4 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/xrpld/app/misc/SHAMapStoreImp.cpp 93.6% 3 Missing ⚠️
src/xrpld/app/ledger/detail/LedgerMaster.cpp 88.9% 1 Missing ⚠️

📢 Thoughts on this report? Let us know!

@ximinez

ximinez commented Jul 10, 2025

Copy link
Copy Markdown
Contributor Author

I've been running this code locally off and on, and one thing I notice is LedgerMaster's validated ledger index is updated before the corresponding ledger is fully updated and available system-wide (i.e. in complete_ledgers). Since the last validated ledger index is used in the gap detection logic as the end of the expected available range, it sees a gap at the end the range and pauses. For example, gap detection is looking for 100-105, but complete ledgers is 50-104.

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 recoveryWaitTime_ from 5 seconds to 1. That's still a lot of time, the check in between takes almost no time, and it will give rotation a lot more time to run in between ledgers.

ximinez added a commit that referenced this pull request Jul 14, 2025
@ximinez
ximinez force-pushed the ximinez/online-delete-gaps branch from b1a5dc1 to e13baa5 Compare July 21, 2025 18:06
@ximinez ximinez added the JiraRefresh Manually trigger Jira automation to run for this item. label Aug 4, 2025
@bthomee bthomee added JiraRefresh Manually trigger Jira automation to run for this item. and removed JiraRefresh Manually trigger Jira automation to run for this item. labels Aug 4, 2025

@xrplf-ai-reviewer xrplf-ai-reviewer Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

No issues.

Review by Claude Sonnet 4.6 · Prompt: V15

@xrplf-ai-reviewer xrplf-ai-reviewer Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good.

Review by Claude Sonnet 4.6 · Prompt: V15

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.

Comment thread src/xrpld/app/ledger/detail/LedgerMaster.cpp
Comment thread src/test/app/LedgerMaster_test.cpp Outdated
Comment thread src/test/app/SHAMapStore_test.cpp Outdated

@xrplf-ai-reviewer xrplf-ai-reviewer Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

No issues.

Review by Claude Sonnet 4.6 · Prompt: V15

@github-actions

This comment has been minimized.

…te-gaps

* XRPLF/develop:
  test: Add tests for check doxygen style (7795)
  style: Add pre-commit hook to check doxygen style (7794)
  style: Unify style for all Doxygen comments (7776)
@github-actions

This comment has been minimized.

@xrplf-ai-reviewer xrplf-ai-reviewer Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

No issues.

Review by Claude Sonnet 4.6 · Prompt: V15

@xrplf-ai-reviewer xrplf-ai-reviewer Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

Review by Claude Sonnet 4.6 · Prompt: V15

@xrplf-ai-reviewer xrplf-ai-reviewer Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

No issues.

Review by Claude Sonnet 4.6 · Prompt: V15

@xrplf-ai-reviewer xrplf-ai-reviewer Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

No issues.

Review by Claude Sonnet 4.6 · Prompt: V15

@xrplf-ai-reviewer xrplf-ai-reviewer Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good.

Review by Claude Sonnet 4.6 · Prompt: V15

@xrplf-ai-reviewer xrplf-ai-reviewer Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

No issues.

Review by Claude Sonnet 4.6 · Prompt: V15

@xrplf-ai-reviewer xrplf-ai-reviewer Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good. No issues to flag.

@xrplf-ai-reviewer xrplf-ai-reviewer Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

One potentially flaky assertion flagged inline — see line 680.

Review by Claude Sonnet 4.6 · Prompt: V15

Comment thread src/test/app/SHAMapStore_test.cpp Outdated

@xrplf-ai-reviewer xrplf-ai-reviewer Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

No issues.

Review by Claude Sonnet 4.6 · Prompt: V15

@xrplf-ai-reviewer xrplf-ai-reviewer Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

No issues.

Review by Claude Sonnet 4.6 · Prompt: V15

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

Labels

JiraRefresh Manually trigger Jira automation to run for this item. Triaged Issue/PR has been triaged for viability, liveliness, etc.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants