-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
chore: blockstore: Plumb through a proper Flush() method on all blockstores #10465
Conversation
This is something @raulk wanted to do for a long time. @ZenGround0 this could be of interest to what you were recently working on, I use it in a separate commit elsewhere as follows: ribasushi/ltsh@e7629792afd |
@@ -18,13 +18,18 @@ type Blockstore interface { | |||
blockstore.Blockstore | |||
blockstore.Viewer | |||
BatchDeleter | |||
Flusher |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know that some external repos depend/implement this blockstore interface, so quite likely this will break some things, but imo that's acceptable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Examples?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RIBS / DagParts, so mostly just my fault 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do i care that much aboout you....?? hrm.
@@ -476,6 +476,23 @@ func (s *SplitStore) GetSize(ctx context.Context, cid cid.Cid) (int, error) { | |||
} | |||
} | |||
|
|||
func (s *SplitStore) Flush(ctx context.Context) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is probably slightly more complicated while a compaction is running, but this impl is still more correct than no flush
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@magik6k @ZenGround0 I do not insist this being the right order: it only seemed correct based on how I read the code ( we use the RLock as an actual lock from what I can tell )
Feel free to rip this method up and implement better!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nothing is obviously broken here though maybe there are some slightly less obvious breakages, let me know if you see that flushing splitstore breaks everything
Proposed Changes
Allow any blockstore to induce a
Flush()
/fsync()
on any underlying storage regardless of amount of indirection.Checklist
Before you mark the PR ready for review, please make sure that:
<PR type>: <area>: <change being made>
fix: mempool: Introduce a cache for valid signatures
PR type
: fix, feat, build, chore, ci, docs, perf, refactor, revert, style, testarea
, e.g. api, chain, state, market, mempool, multisig, networking, paych, proving, sealing, wallet, deps