-
Notifications
You must be signed in to change notification settings - Fork 180
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
[Storage] Pruning - add a test case to verify compaction can reclaim disk space #6603
Conversation
b5cb99c
to
402c14d
Compare
257bde0
to
bbfb3a1
Compare
402c14d
to
3e53a01
Compare
require.Greater(t, sizeBefore, sizeAfter, | ||
fmt.Sprintf("expected disk usage to be reduced after compaction, before: %d, after: %d", sizeBefore, sizeAfter)) |
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 verifies the disk usage will be reduced.
77b44d1
to
17972e0
Compare
3e53a01
to
e25393f
Compare
207cd1a
to
77fb95b
Compare
77fb95b
to
5b15c09
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6603 +/- ##
==========================================
- Coverage 41.18% 41.17% -0.02%
==========================================
Files 2109 2111 +2
Lines 185660 185733 +73
==========================================
+ Hits 76466 76467 +1
- Misses 102781 102852 +71
- Partials 6413 6414 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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 mostly focused on tests and left a minor suggestion.
storage/operation/writes_test.go
Outdated
|
||
// Trigger compaction | ||
require.NoError(t, db.Compact(prefix, []byte{2}, true)) | ||
wg.Wait() |
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.
Maybe use a timer here so the test doesn't stuck on wg.Wait()
in case of error.
require.NoError(t, db.Compact(prefix, []byte{2}, true)) | ||
|
||
// Use a timer to implement a timeout for wg.Wait() | ||
timeout := time.After(30 * 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.
this sounds like a long time... can we shorten the timer.
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.
Will adjust in a separate PR
Review this PR first
Working towards #6516