feat: add cleanup test, and isolate id mapping code and cleanup code #218
Conversation
| cancel() | ||
| }() | ||
|
|
||
| ticker := time.NewTicker(cs.cfg.interval) |
There was a problem hiding this comment.
Move this until after the first call to cs.clenaup(ctx), to line 57, so that the timer does not start until after the first cleanup is done.
There was a problem hiding this comment.
I did it in this order because after the initial call, the behavior of the ticker is to tick x duration after the start of the previous cleanup, not after the end of the previous cleanup. so this way the behavior stays consistent with the regular loop, and cleanups will happen at predictable times.
if it is desired to start waiting for the duration after the previous cleanup finishes instead of immediately after it starts, we would probably have to replace the ticker with a timer that is manually scheduled at the end of a cleanup. lmk what u think
There was a problem hiding this comment.
OK, I see.
Would be nice to put the defer ticker.Stop() right after the ticker is created to make clear it is stopped at exit.
| // func (im *idMap) remove(blobID blob.ID) error { | ||
| // if err := os.Remove(im.path(blobID)); err != nil { | ||
| // if errors.Is(err, os.ErrNotExist) { | ||
| // return blob.ErrBlobNotFound |
There was a problem hiding this comment.
Would it fix the warning by not returning an error here? It is reasonable to not return file-not-found error when removing a file, since it is already removed.
There was a problem hiding this comment.
the error isn't related to the return type, the function just isn't called anywhere yet and the linter doesn't like private functions that are unused
There was a problem hiding this comment.
alternatively, i could make this struct and its methods public to get rid of the warning? does it make sense to be public though?
There was a problem hiding this comment.
Let's not export (make public) anything unless really needed. Otherwise, it risks becoming an API we need to support.
Fine like it is then
integration/singularity/id_map.go
Outdated
| } | ||
|
|
||
| func (im *idMap) path(blobID blob.ID) string { | ||
| return path.Join(im.dir, blobID.String()+".id") |
There was a problem hiding this comment.
Can im.dir be a filesystem path (possibly containing system-specific delimiters)? If yes, that it may be necessary to use filepath.Join.
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #218 +/- ##
==========================================
+ Coverage 40.17% 44.00% +3.83%
==========================================
Files 7 9 +2
Lines 941 1009 +68
==========================================
+ Hits 378 444 +66
+ Misses 515 513 -2
- Partials 48 52 +4
|
| cancel() | ||
| }() | ||
|
|
||
| ticker := time.NewTicker(cs.cfg.interval) |
There was a problem hiding this comment.
OK, I see.
Would be nice to put the defer ticker.Stop() right after the ticker is created to make clear it is stopped at exit.
| // func (im *idMap) remove(blobID blob.ID) error { | ||
| // if err := os.Remove(im.path(blobID)); err != nil { | ||
| // if errors.Is(err, os.ErrNotExist) { | ||
| // return blob.ErrBlobNotFound |
There was a problem hiding this comment.
Let's not export (make public) anything unless really needed. Otherwise, it risks becoming an API we need to support.
Fine like it is then
elijaharita
force-pushed
the
feat/cleanup-test
branch
from
c6e79d9
to
d9083b8
Compare
elijaharita
force-pushed
the
feat/cleanup-test
branch
from
e5edad4
to
7adcc08
Compare
Wrote a test for the cleanup code while investigating #201. This involved moving cleanup code to its own file. I also used this opportunity to isolate ID mapping code.
The test is passing from the start, so I'm not sure if it'll fix that issue, but there's a possibility. In any case, it cleans up a bit of mess.
The cleanup scheduler is a new object with the sole purpose of reading through the local bin store and removing files based on the result of a configurable callback, at some interval. A configurable callback allows it to be mocked easily for the test. In
store.go, the callback is set to thehasDealForAllProviders()function. This should be tested next. It'll be a bit more involved since it'll have to be added to the integration test, since singularity has to be running, but I think I know how I'll do it.A few additional functions have been added to local store:
List()andDelete(). These were previously done inline instore.goby manually working with the store directory files, which was kinda hacky. This puts that stuff where it belongs.About the new ID map object: a separation of concerns refactor. Motion maps blob IDs to Singularity IDs by storing files ending in ".id" in the same directory, named with the stringified motion ID, and containing the Singularity ID in the body. This processing was all done inline in
store.goWith these changes, this behavior is unmodified, so there should not be any compatibility issues. I originally wrote this because I thought I would be working with IDs incleanup.go, but my final solution did not end up needing it. So in the end it is just a general improvement. It does pave the way for an ID mapping test.should fix #201, awaiting confirmation by @Angelo-gh3990