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

feat:splitstore:single compaction that can handle prune aka two marksets one compaction #9571

Merged
merged 9 commits into from
Nov 7, 2022

Conversation

ZenGround0
Copy link
Contributor

Related Issues

Discussion kicked off here: #9128

Proposed Changes

Prune mode is now just a 3rd mode next to discard and universal. There is only ever one compaction running so no need to add 3 new knobs to splitstore config.

Additional Info

A quick conversation with @TippyFlitsUK supports the hypothesis that this minimal configuration strategy is a better all around approach.

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
  • New features have usage guidelines and / or documentation updates in
  • Tests exist for new functionality or change in behavior
  • CI is green

@ZenGround0 ZenGround0 marked this pull request as ready for review November 2, 2022 11:43
@ZenGround0 ZenGround0 requested a review from a team as a code owner November 2, 2022 11:43
@ZenGround0 ZenGround0 requested review from vyzo and magik6k November 2, 2022 11:43
Copy link
Contributor

@vyzo vyzo left a comment

Choose a reason for hiding this comment

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

This looks like a very reasonable change to me.

I will have to reload some context for a more thorough review, but dont block on me if the magik man greens.

itests/kit/node_opts.go Outdated Show resolved Hide resolved
blockstore/splitstore/splitstore_compact.go Outdated Show resolved Hide resolved
blockstore/splitstore/splitstore_compact.go Show resolved Hide resolved
blockstore/splitstore/splitstore_compact.go Show resolved Hide resolved
@ZenGround0 ZenGround0 requested a review from magik6k November 7, 2022 12:03
Copy link
Contributor

@magik6k magik6k left a comment

Choose a reason for hiding this comment

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

This looks correct to me, but one more set of 👀 on this before merging would be really nice

Comment on lines -1299 to 1342
func (s *SplitStore) deadSetPath() string {
func (s *SplitStore) discardSetPath() string {
return filepath.Join(s.path, "deadset")
Copy link
Contributor

Choose a reason for hiding this comment

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

Just sanity checking - we're not un-renaming the function because deadSet is now not quite what it used to mean?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah it now has a use in compaction not just prune and this is better suited to that domain.

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.

4 participants