nsq_to_file: new flag -rotate-check-interval #1071
Conversation
FileLogger checks whether a file should be rotated every rotate-check-interval duration (default 30s).
|
It's worth noting that case <-ticker.C:
if f.needsFileRotate() {
...
case m := <-f.logChan:
if f.needsFileRotate() {
...So the ticker, and this change, just affects how quickly the "last" log file is closed when there are no new messages, and the generation of empty files. |
|
I should probably stamp that stable release before landing this? |
mreiferson
changed the title
|
Ah, so this is for the Yeah, I think we can stamp the stable release, and then merge this and the other PRs you've revived recently. |
|
thanks again @alpaker! |
| @@ -34,6 +34,7 @@ var ( | |||
|
|
|||
| rotateSize = flag.Int64("rotate-size", 0, "rotate the file when it grows bigger than `rotate-size` bytes") | |||
| rotateInterval = flag.Duration("rotate-interval", 0*time.Second, "rotate the file every duration") | |||
| rotateCheckInterval = flag.Duration("rotate-check-interval", 30*time.Second, "check whether the file should be rotated every duration") | |||
There was a problem hiding this comment.
I think name and description is not really right - the check whether the file should be rotated is done on every single message, this really just affects how often the files are synced to disk (including empty files I guess)
There was a problem hiding this comment.
Hmmmm, what do you have in mind?
There was a problem hiding this comment.
I think "sync-interval" makes sense. description is a bit harder ... "sync current file on disk every duration" ... "(also check if file should should have been rotated since the last message received)"
There was a problem hiding this comment.
Yea, I agree, --sync-interval makes more sense. I can whip that up or do you wanna do the honors @alpaker?
There was a problem hiding this comment.
Sure. Regarding the description: If we're going to foreground the flush interval as what's controlled by this option, then I suggest omitting mention of rotation altogether. That the need for rotation is checked on msg receipt isn't explicitly documented, which makes the significance of "check if file should should have been rotated since the last message received" unclear. How about:
syncInterval = flag.Duration("sync-interval", 30*time.Second, "flush file to disk every duration")
There was a problem hiding this comment.
Sounds good, except s/flush/sync to match the flag name?
There was a problem hiding this comment.
all looks good to me, I was just trying to appease the reasoning behind the original naming :)
This PR adds a new CLI option
rotate-check-interval(default 30s) tonsq_to_filethat controls the frequency at which FileLogger checks whether a file should be rotated. Fixes #1070.