Conversation
|
This is now verified to address the earlier mentioned issues of rotation based on size or time not working? |
solatis
left a comment
solatis
left a comment
There was a problem hiding this comment.
Pre-approved, minor comments.
One question I would have is future compatibility -- we effectively vendored and started customizing the lumberjack library, is that correct? Is there a better approach that does not require us to vendor this library yet still be able to customize this behavior?
| // MaxAge is the maximum number of seconds to retain old log files based on the | ||
| // timestamp encoded in their filename. | ||
| // MaxAge is the maximum number of seconds before log file would be rotated. | ||
| //We support rotation on time and on size. |
There was a problem hiding this comment.
Indentation incorrect, missing a space
| millCh chan bool | ||
| startMill sync.Once | ||
|
|
||
| lastTimeRotate time.Time |
There was a problem hiding this comment.
What is this initialized as?
Do I also understand correctly that we now effectively both vendored but also customized the lumberjack logging library? How will we handle future updates / bugfixes by upstream?
There was a problem hiding this comment.
@solatis you are right we did make changes to lumberjack lib and we significantly changed its behavior for MaxAge param. Unfortunately all updates from upstream needs to be manually validated and manually added to our repo.
|
@igorniebylski or @rodp63 or @terngkub -- can you QA this before we merge it into master? |
|
LGTM. @solatis after each log entry is written, the rotation conditions (size and time) are evaluated. |
3 participants
Changes for log rotation to address issue found in QA.
After that change we support log rotation based on time and size.
Priority for rotations: