Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

experiment:splitstore:No redundant delete on moving GC #11880

Closed
wants to merge 2 commits into from

Conversation

ZenGround0
Copy link
Contributor

Related Issues

#11778

Proposed Changes

@ribasushi points out that we do a full cold deletion purge in the event we are going to throw out the entire coldstore entirely anyway. This is particularly wasteful when moving GC frequency is 1.

I don't want to skip the purging process entirely because missing the criticial section opens up serious safety problems. That's the "right" way to do this. But let's see if this simple and trivially safe change can get us 80% of the way there.

Additional Info

Checklist

Before you mark the PR ready for review, please make sure that:

  • Commits have a clear commit message.
  • PR title is in the form of of <PR type>: <area>: <change being made>
    • example: fix: mempool: Introduce a cache for valid signatures
    • PR type: fix, feat, build, chore, ci, docs, perf, refactor, revert, style, test
    • area, e.g. api, chain, state, market, mempool, multisig, networking, paych, proving, sealing, wallet, deps
  • If the PR affects users (e.g., new feature, bug fix, system requirements change), update the CHANGELOG.md and add details to the UNRELEASED section.
  • New features have usage guidelines and / or documentation updates in
  • Tests exist for new functionality or change in behavior
  • CI is green

@ribasushi
Copy link
Contributor

Included patch in 2 production nodes, will report how it looks tomorrow

can get us 80% of the way there.

See #11778 (comment)

@ribasushi
Copy link
Contributor

@ZenGround0 🙅 , do not continue with this PR.

The comment here is a lie: https://github.com/filecoin-project/lotus/blob/v1.26.2/blockstore/badger/blockstore.go#L392-L393 . No filtering capability is implemented, let alone a filter being passed. This means the the entire "old store" is copied as-is to the new location. If we do not Delete() stuff beforehand, no reduction of the k/v-set takes place 🤦

@ZenGround0 ZenGround0 closed this Apr 24, 2024
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.

None yet

2 participants