Verify checkpoint files are completely written out when checking if they exist #1086
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1086 +/- ##
==========================================
+ Coverage 75.3% 77.32% +2.02%
==========================================
Files 569 578 +9
Lines 48213 48462 +249
==========================================
+ Hits 36307 37475 +1168
+ Misses 9640 8627 -1013
- Partials 2266 2360 +94
Continue to review full report at Codecov.
|
richardartoul
force-pushed
the
ra/fix-exists
branch
from
564c183 to
58ab6ec
Compare
| // CompleteCheckpointFileExists returns whether a checkpoint file exists, and if so, | ||
| // is it complete. | ||
| func CompleteCheckpointFileExists(filePath string) (bool, error) { | ||
| f, err := os.Stat(filePath) |
There was a problem hiding this comment.
should this have the same strings.Contains check?
There was a problem hiding this comment.
what would the check be? This one is specifically written to be used for checkpoint files, and if its not a checkpoint file you'll probably get an error anyways because its unlikely the file you're checking is exactly 4 bytes
There was a problem hiding this comment.
I think prateek just means, should you check that the filepath they're checking for definitely is a checkpoint file (i.e. make sure that it passes the checkpoint suffix)? I think that makes sense.
There was a problem hiding this comment.
Ah ok, I guess I just wasn't worried about it because this function is harder to misuse but I'll add it
| start := time.Now() | ||
| shardDir := ShardDataDirPath(dir, testNs1ID, shard) | ||
| err := os.MkdirAll(shardDir, defaultNewDirectoryMode) | ||
| var ( |
There was a problem hiding this comment.
mind making a new test which only tests the checkpointFileSizeBytes condition we're worried about and adding a little blurb about why it exists.
There was a problem hiding this comment.
Added a test and put a comment in the function itself
src/dbnode/persist/fs/write.go
Outdated
| @@ -40,6 +40,10 @@ import ( | |||
| xtime "github.com/m3db/m3x/time" | |||
| ) | |||
|
|
|||
| const ( | |||
| checkpointFileSizeBytes = 4 | |||
There was a problem hiding this comment.
maybe have this and digest/digestLen be exported types and ensure they're the same in a test.
| func TestCheckpointFileSizeBytesSize(t *testing.T) { | ||
| // These values need to match so that the logic for determining whether | ||
| // a checkpoint file is complete or not remains correct. | ||
| require.Equal(t, digest.DigestLenBytes, CheckpointFileSizeBytes) |
There was a problem hiding this comment.
Instead of this, why don't you just make CheckpointFileSizeBytes = digest.DigestLenBytes? I actually don't mind either way though, whatever works.
There was a problem hiding this comment.
Yeah I discussed this with Prateek and we like the idea that the tests would break if you changed one and not the other just to make the person think about the change more
The existing implementation would assume that if a checkpoint file existed on disk that everything was fine, but we found that in some cases it was possible to end up with a checkpoint file of size 0 (no digest). In that case, we want to proceed as if the checkpoint file did not exist as it was not completely written out.