-
Notifications
You must be signed in to change notification settings - Fork 928
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(shwap/store): Add q4 trimming support in store #3680
Conversation
4bbe418
to
833f194
Compare
833f194
to
06b6535
Compare
return | ||
} | ||
if ctx.Err() != nil { | ||
ctx = context.Background() |
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.
So, always blocking? Maybe context.WithTimeout(context.Background(), time.Second)
?
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.
Good that you pointed out. We use common function to reset context in metrics everywhere else. I'll change how we do it in another PR.
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.
Agree. It may cause random hickups somewhere in the future that would be a nightmare to debug
06b6535
to
d572276
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## shwap #3680 +/- ##
========================================
Coverage ? 46.63%
========================================
Files ? 314
Lines ? 17792
Branches ? 0
========================================
Hits ? 8298
Misses ? 8490
Partials ? 1004 ☔ View full report in Codecov by Sentry. |
store/store.go
Outdated
@@ -305,51 +304,76 @@ func (s *Store) hasByHeight(height uint64) (bool, error) { | |||
return exists(pathODS) | |||
} | |||
|
|||
func (s *Store) Remove(ctx context.Context, height uint64, datahash share.DataHash) error { | |||
func (s *Store) RemoveAll(ctx context.Context, height uint64, datahash share.DataHash) 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.
Let's name to RemoveODSQ4
. Its not clear what All stands for.
Same for helpers methods and metrics.
store/store.go
Outdated
@@ -33,7 +33,7 @@ const ( | |||
var ErrNotFound = errors.New("eds not found in store") | |||
|
|||
// Store is a storage for EDS files. It persists EDS files on disk in form of Q1Q4 files or ODS | |||
// files. It provides methods to put, get and remove EDS files. It has two caches: recent eds cache | |||
// files. It provides methods to put, get and removeAll EDS files. It has two caches: recent eds cache |
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.
rename relic
return | ||
} | ||
if ctx.Err() != nil { | ||
ctx = context.Background() |
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.
Agree. It may cause random hickups somewhere in the future that would be a nightmare to debug
No description provided.