-
Notifications
You must be signed in to change notification settings - Fork 265
feat(chainstate): Add json persister #2495
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
base: indexer-memstore
Are you sure you want to change the base?
Changes from all commits
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 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,90 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| package store | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import ( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "context" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "fmt" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "os" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "time" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "github.com/Layr-Labs/eigensdk-go/logging" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // JSONPersister handles periodic persistence of store state to a JSON file. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| type JSONPersister struct { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| store Store | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| path string | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| logger logging.Logger | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // NewJSONPersister creates a new JSON persister for the given store. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| func NewJSONPersister(store Store, path string, logger logging.Logger) *JSONPersister { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
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. Should we call
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. Alternatively this is a problem on the infra side to make sure directories are setup properly. I'm okay with either way. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return &JSONPersister{ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| store: store, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| path: path, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| logger: logger, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Save persists the current store state to the configured JSON file. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // It uses atomic file operations (write to temp, then rename) to ensure consistency. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| func (p *JSONPersister) Save(ctx context.Context) error { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| data, err := p.store.Snapshot() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if err != nil { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return fmt.Errorf("failed to create snapshot: %w", err) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| tmpPath := p.path + ".tmp" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if err := os.WriteFile(tmpPath, data, 0644); err != nil { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if err := os.WriteFile(tmpPath, data, 0644); err != nil { | |
| if err := os.WriteFile(tmpPath, data, 0600); err != nil { |
Copilot
AI
Feb 10, 2026
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.
If the temporary file write succeeds but the rename operation fails, the temporary file will be left behind. Consider adding cleanup logic to remove the temporary file on error, or document that orphaned .tmp files may need to be cleaned up after failures.
| if err := os.Rename(tmpPath, p.path); err != nil { | |
| if err := os.Rename(tmpPath, p.path); err != nil { | |
| // Best-effort cleanup of the temporary file if rename fails. | |
| if removeErr := os.Remove(tmpPath); removeErr != nil && !os.IsNotExist(removeErr) { | |
| p.logger.Error("Failed to remove temp state file after rename error", "path", tmpPath, "error", removeErr) | |
| } |
Copilot
AI
Feb 10, 2026
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.
The codebase has an established AtomicWrite utility function at litt/util/file_utils.go that provides better atomic file writing guarantees than the current implementation. It handles fsync of both the file and parent directory, and uses proper cleanup of temporary files. Consider using this utility instead of manually implementing atomic writes to maintain consistency across the codebase and improve durability guarantees.
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.
nit: Can we make this debug log and emit an info log on final save?
Copilot
AI
Feb 10, 2026
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.
The context parameter passed to Save is not used. If the intention is to respect context cancellation during the save operation, consider checking ctx.Done() before performing the save, or remove the parameter if it's not needed. The final save in StartPeriodicSave uses context.Background(), which bypasses cancellation that might be in progress.
Copilot
AI
Feb 10, 2026
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.
The context parameter passed to Load is not used within the function. Consider either using it to respect cancellation (e.g., checking ctx.Done() before the restore operation) or removing it if context support isn't needed for this operation.
Copilot
AI
Feb 10, 2026
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.
StartPeriodicSave returns immediately without providing a way for the caller to know when the background goroutine has finished or to wait for the final save to complete. Consider returning a channel or using a wait group to allow the caller to synchronize with the completion of the final save operation before shutdown completes.
| func (p *JSONPersister) StartPeriodicSave(ctx context.Context, interval time.Duration) { | |
| ticker := time.NewTicker(interval) | |
| defer ticker.Stop() | |
| for { | |
| select { | |
| case <-ticker.C: | |
| if err := p.Save(ctx); err != nil { | |
| p.logger.Error("Failed to persist state", "error", err) | |
| } | |
| case <-ctx.Done(): | |
| // Perform final save before shutdown | |
| p.logger.Info("Context cancelled, performing final state save") | |
| if err := p.Save(context.Background()); err != nil { | |
| p.logger.Error("Failed final state save", "error", err) | |
| } | |
| return | |
| } | |
| } | |
| // The returned channel is closed when the background goroutine has finished, | |
| // allowing callers to wait for the final save to complete before shutdown. | |
| func (p *JSONPersister) StartPeriodicSave(ctx context.Context, interval time.Duration) <-chan struct{} { | |
| done := make(chan struct{}) | |
| go func() { | |
| defer close(done) | |
| ticker := time.NewTicker(interval) | |
| defer ticker.Stop() | |
| for { | |
| select { | |
| case <-ticker.C: | |
| if err := p.Save(ctx); err != nil { | |
| p.logger.Error("Failed to persist state", "error", err) | |
| } | |
| case <-ctx.Done(): | |
| // Perform final save before shutdown | |
| p.logger.Info("Context cancelled, performing final state save") | |
| if err := p.Save(context.Background()); err != nil { | |
| p.logger.Error("Failed final state save", "error", err) | |
| } | |
| return | |
| } | |
| } | |
| }() | |
| return done |
Copilot
AI
Feb 10, 2026
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.
There are no tests for the JSONPersister implementation. The repository has comprehensive test coverage across other modules. Consider adding tests to cover: successful Save/Load operations, handling of non-existent files in Load, atomic write behavior, StartPeriodicSave functionality with context cancellation, and error cases like write failures or corrupt JSON data during restoration.
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.
isn't a JSON persister just a snapshot persister?