-
Notifications
You must be signed in to change notification settings - Fork 162
feat(statesync): introduce Finalizer interface for syncer cleanup #1422
base: master
Are you sure you want to change the base?
Changes from all commits
6700e62
3b38cf0
83315e1
bbe4384
3ac161f
00e8084
26f9713
e953c21
ee80793
23ab197
d7ec158
2219e78
26a2c5a
7fc9a87
c3e1590
b0565c7
cf30dfe
ee19498
5131d0c
626b3fb
69fb806
c0d0e5b
c21d924
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -37,7 +37,6 @@ type LeafSyncTask interface { | |||||||||||||||
| type LeafSyncerConfig struct { | ||||||||||||||||
| RequestSize uint16 // Number of leafs to request from a peer at a time | ||||||||||||||||
| NumWorkers int // Number of workers to process leaf sync tasks | ||||||||||||||||
| OnFailure func() // Callback for handling errors during sync | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| type CallbackLeafSyncer struct { | ||||||||||||||||
|
|
@@ -159,9 +158,9 @@ func (c *CallbackLeafSyncer) Sync(ctx context.Context) error { | |||||||||||||||
| }) | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| err := eg.Wait() | ||||||||||||||||
| if err != nil { | ||||||||||||||||
| c.config.OnFailure() | ||||||||||||||||
| if err := eg.Wait(); err != nil { | ||||||||||||||||
| return err | ||||||||||||||||
| } | ||||||||||||||||
| return err | ||||||||||||||||
|
|
||||||||||||||||
| return nil | ||||||||||||||||
|
Comment on lines
+161
to
+165
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||||
| } | ||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -106,7 +106,6 @@ func NewSyncer(client syncclient.Client, db ethdb.Database, root common.Hash, co | |
| ss.syncer = syncclient.NewCallbackLeafSyncer(client, ss.segments, &syncclient.LeafSyncerConfig{ | ||
| RequestSize: leafsRequestSize, | ||
| NumWorkers: defaultNumWorkers, | ||
| OnFailure: ss.onSyncFailure, | ||
| }) | ||
|
|
||
| if codeQueue == nil { | ||
|
|
@@ -301,19 +300,19 @@ func (t *stateSync) removeTrieInProgress(root common.Hash) (int, error) { | |
| return len(t.triesInProgress), nil | ||
| } | ||
|
|
||
| // onSyncFailure is called if the sync fails, this writes all | ||
| // batches of in-progress trie segments to disk to have maximum | ||
| // progress to restore. | ||
| func (t *stateSync) onSyncFailure() { | ||
| // Finalize checks if there are any in-progress tries and flushes their batches to disk | ||
| // to preserve progress. This is called by the syncer registry on sync failure or cancellation. | ||
| func (t *stateSync) Finalize() error { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Based off the name alone, it seems like this shouldn't occur on success
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this is ok, but you should update the comments |
||
| t.lock.RLock() | ||
| defer t.lock.RUnlock() | ||
|
|
||
| for _, trie := range t.triesInProgress { | ||
| for _, segment := range trie.segments { | ||
| if err := segment.batch.Write(); err != nil { | ||
| log.Error("failed to write segment batch on sync failure", "err", err) | ||
| return | ||
| log.Error("failed to write segment batch on finalize", "err", err) | ||
| return err | ||
| } | ||
| } | ||
| } | ||
| return nil | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -31,6 +31,13 @@ type Syncer interface { | |
| ID() string | ||
| } | ||
|
|
||
| // Finalizer provides a mechanism to perform cleanup operations after a sync operation. | ||
| // This is useful for handling inflight requests, flushing to disk, or other cleanup tasks. | ||
| type Finalizer interface { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's interesting you made this a separate interface. I totally agree that we shouldn't force all syncers to adhere to this, but should we instead make it something like: I'm honestly not sure, what do you think? My only concern with the current approach is that it seems unintuitive to check the types all the time, and rather we would expect the caller to either dynamic sync everything or static sync everything, right?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I actually now agree we this. This seems necessary for the state syncer, for example. Should it take in an error? (e.g. handle the cleanup different in the error vs no error case?)
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I also think you should add this sort of comment on the syncer registry, since it doesn't say it will call finalize |
||
| // Finalize performs any necessary cleanup operations. | ||
| Finalize() error | ||
| } | ||
|
|
||
| // SummaryProvider is an interface for providing state summaries. | ||
| type SummaryProvider interface { | ||
| StateSummaryAtBlock(ethBlock *types.Block) (block.StateSummary, error) | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
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.
Why would db be nil?