Close WAL block file before deleting everything #2152
Conversation
kostya9
requested review from
joe-elliott,
annanay25,
mdisibio,
mapno,
kvrhdn and
zalegrala
as code owners
|
|
| @@ -460,6 +463,8 @@ func (b *walBlock) FindTraceByID(ctx context.Context, id common.ID, _ common.Sea | |||
| } | |||
|
|
|||
| r := parquet.NewReader(file) | |||
| defer r.Close() | |||
There was a problem hiding this comment.
should we also defer close the file above? and all of the files returned from page.file() calls?
There was a problem hiding this comment.
parquet.File unfortunately does not implement a Close function, so we can not close it. I noticed that when I was figuring out what is going on and logged an issue - #2153
There was a problem hiding this comment.
oh, i forgot this was a parquet.File and not an os.File. wdyt about returning a close() method from file() to close the underlying os file?
There was a problem hiding this comment.
Sounds interesting.
What do you think of wrapping the parquet.File with another type that implements Close()? I feel like maybe that would be a more intuitive design. Also, it could later be used in other call sites theoretically (which would probably also need to close the file at some point).
There was a problem hiding this comment.
so like:
type pageFile struct {
parquet.File
os.File
}
then the caller can interact with the parquet.file and eventually close the os.file? i'm good on that
There was a problem hiding this comment.
Added closing the file opened in file(). It resulted in a little bit more changes that I anticipated, but we should close everything opened in the wal_block module now
There was a problem hiding this comment.
Ah, did all that work, but didn't call the final iterator.Close(). Fixed that
kostya9
force-pushed
the
close_files_after_opening
branch
from
14268cc
to
19a894c
Compare
| @@ -460,6 +463,8 @@ func (b *walBlock) FindTraceByID(ctx context.Context, id common.ID, _ common.Sea | |||
| } | |||
|
|
|||
| r := parquet.NewReader(file) | |||
| defer r.Close() | |||
There was a problem hiding this comment.
oh, i forgot this was a parquet.File and not an os.File. wdyt about returning a close() method from file() to close the underlying os file?
There was a problem hiding this comment.
@mapno I'm good on this, but before merging can you take a quick look? this does a lot of subtle changes with regard to the wal and searching. i'm concerned we may see some strange issues crop up b/c we relied on these files being opened, but i think explicitly closing these files is the right direction.
don't feel the need to do a line by line review (unless you want). mainly flagging you due to the changes along the SearchTags and TagValues paths.
@kostya9 thanks for the persistence and level of detail on this! after @mapno takes a look we will merge.
…ff from the directory Close other files after opening.
kostya9
force-pushed
the
close_files_after_opening
branch
from
a498555
to
529be34
Compare
…MetadataIterator (wrap it instead of spansetIterator)
kostya9
force-pushed
the
close_files_after_opening
branch
from
529be34
to
a3136d7
Compare
|
I see failures in the generator storage tests, from my analysis, the generator doesn't use the code I changed here. Are these tests flaky, or is there a chance something is broken? |
Yeah. It's a flakey test. Will rerun. |
|
Thanks for the fixes @kostya9! |
* WAL_BLOCK should close the file it opened before trying to remove stuff from the directory Close other files after opening. * Add changelog row * Add nullcheck * Add one more missed close * Aggregate errors for wal.Clear * Close file opened by WAL's file() by wrapping the parquet.File with os.File * Add the acual close for the iterator * Fix changelog entry position. Fix types after introduction of spansetMetadataIterator (wrap it instead of spansetIterator)
What this PR does: Close WAL block file before deleting everything
Also added some other files closing that I found during a look around the code.
PS: I not proficient in Go, so I could have misunderstood, so please check if it looks good
Which issue(s) this PR fixes:
Attempt to fix #2034
Fixes #2153
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]