Avoid splitting log line across two files #4282
Conversation
BenchmarkRotator/1KB-8 200000 5572 ns/op BenchmarkRotator/2KB-8 200000 8338 ns/op BenchmarkRotator/4KB-8 100000 14246 ns/op BenchmarkRotator/8KB-8 50000 25279 ns/op BenchmarkRotator/16KB-8 30000 48602 ns/op BenchmarkRotator/32KB-8 20000 92159 ns/op BenchmarkRotator/64KB-8 10000 154766 ns/op BenchmarkRotator/128KB-8 5000 296872 ns/op BenchmarkRotator/256KB-8 3000 551793 ns/op
We attempt to avoid splitting a log line between two files by detecting if we are near the file size limit and scanning for new lines and only flushing those. BenchmarkRotator/1KB-8 300000 5613 ns/op BenchmarkRotator/2KB-8 200000 8384 ns/op BenchmarkRotator/4KB-8 100000 14604 ns/op BenchmarkRotator/8KB-8 50000 25002 ns/op BenchmarkRotator/16KB-8 30000 47572 ns/op BenchmarkRotator/32KB-8 20000 92080 ns/op BenchmarkRotator/64KB-8 10000 165883 ns/op BenchmarkRotator/128KB-8 5000 294405 ns/op BenchmarkRotator/256KB-8 2000 572374 ns/op
| // Scan for new line and if the data up to new line fits in current | ||
| // file, write to buffer | ||
| idx := bytes.IndexByte(p[n:], newLineDelimiter) | ||
| if idx >= 0 && (remainingSpace-int64(idx)-1) >= 0 { |
There was a problem hiding this comment.
Does this conditional work right if the first char in p is a new line, i.e idx=0?
IIUC it does work, It would write just the new line char to the buffer and then get back to the for loop.
Might want to make a unit test for such edge cases though?
There was a problem hiding this comment.
Yeah that is what the minus 1 is for. Since a write of p[n:n] actually writes 1 byte so I have to check if there is room for whatever index + 1
| // 2nd: Write long string with no new lines | ||
| expectations := [][]byte{ | ||
| []byte("ab\n"), | ||
| []byte("cdef\n"), |
There was a problem hiding this comment.
would suggest adding test cases with new line in the beginning, and middle of string
There was a problem hiding this comment.
str1 captures the middle but I can add one for the beginning
|
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
Fixes #1048