feat(mongodb-log-writer): add retentionGB configuration MONGOSH-1985
#504
Conversation
gagik
changed the title
logRetentionGB configuration MONGOSH-1985retentionGB configuration MONGOSH-1985
gagik
force-pushed
the
gagik/log-writer-gb-retention
branch
from
d7a3e25 to
3f326b0
Compare
gagik
force-pushed
the
gagik/log-writer-gb-retention
branch
from
3f326b0 to
6e4e929
Compare
| leastRecentFileHeap.size() > this._options.maxLogFileCount; | ||
|
|
||
| if (reachedMaxStorageSize || reachedMaxFileCount) { | ||
| toDelete = leastRecentFileHeap.pop(); |
There was a problem hiding this comment.
So ... took some time to think this through, but the problem with this approach is that we'll generally want to delete the oldest files that are necessary to ensure that max file count + max file size limitations
But with this approach, that's no longer the case – you could end up with e.g. the most recent file being deleted, if it happens to be the first file we read from the directory and it's already over the size limit – right?
(I'm not sure if we need to let go of the heap as the underlying data structure, certainly wouldn't be the end of the world, but I'm pretty sure that the "read one file, check conditions, maybe delete one file" approach won't work anymore)
There was a problem hiding this comment.
hmm yeah, we could get rid of the heap, sort by creation date (which would be same as the log's name?) from the getgo and adjust the logic accordingly
There was a problem hiding this comment.
which would be same as the log's name?
Yes, that's an assumption I'd still keep around 👍
gagik
force-pushed
the
gagik/log-writer-gb-retention
branch
from
8b8c74e to
b008854
Compare
| @@ -17,9 +16,11 @@ interface MongoLogOptions { | |||
| retentionDays: number; | |||
| /** The maximal number of log files which are kept. */ | |||
| maxLogFileCount?: number; | |||
| /** A handler for warnings related to a specific filesystem path. */ | |||
| onerror: (err: Error, path: string) => unknown | Promise<void>; | |||
| /** The maximal GB of log files which are kept. */ | |||
There was a problem hiding this comment.
| /** The maximal GB of log files which are kept. */ | |
| /** The maximal size of log files which are kept. */ |
| const files = await fs.readdir(dir, { withFileTypes: true }); | ||
| for (const file of files) { |
There was a problem hiding this comment.
Why not .opendir(), which properly streams, if we're iterating it in a loop anyway?
There was a problem hiding this comment.
oh yeah, I originally had a reduce-based logic (which is also how the sort got lost...) which is why I switched to read, will switch back
| } | ||
|
|
||
| sortedLogFiles.push({ fullPath, id, size }); | ||
| } |
There was a problem hiding this comment.
This being a loop means we should be checking maxDurationMs now here as well
It's also a bit unfortunate that if this part takes longer than the maximum duration, we now don't end up deleting files even if we have already identified that they should be deleted regardless of that (e.g. through the expiration time setting or the max file count setting)
| ? this._options.retentionGB * 1024 * 1024 * 1024 | ||
| : Infinity; | ||
|
|
||
| for await (const { id, fullPath } of [...sortedLogFiles]) { |
There was a problem hiding this comment.
I'm a bit confused – Where is sortedLogFiles being sorted?
gagik
force-pushed
the
gagik/log-writer-gb-retention
branch
3 times, most recently
from
5bc4cb7 to
b219436
Compare
| } | ||
| } | ||
|
|
||
| if (this._options.retentionGB) { |
There was a problem hiding this comment.
I think this should address most of the concerns.
Not sure what other issues might be with fs.stat worth handling
Includes test for random OS-based file order as well as mixing different settings together.
gagik
force-pushed
the
gagik/log-writer-gb-retention
branch
from
b219436 to
99afade
Compare
gagik
force-pushed
the
gagik/log-writer-gb-retention
branch
from
96537b0 to
9f3c1bf
Compare
Adds
retentionGBwhich enforces maximal GB of log files which are kept.