-
Notifications
You must be signed in to change notification settings - Fork 3.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: Log early abort. Fixes #9573 #9575
fix: Log early abort. Fixes #9573 #9575
Conversation
Signed-off-by: pengfei.ji <[email protected]>
//on old version k8s, the line may contains no space, hence len(parts) would equal to 1 | ||
content := "" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How old is your k8s cluster?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1.18
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's too old so we should not support it. https://endoflife.date/kubernetes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I see.
But, in my opinion, it may still be necessary to ensure that the length of parts is greater than 1 here.
If there is a very very long token, which doesn't contain any space in it, the length of parts would equal to 1, thus it
would cause a crash.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sounds like additional scan that could be time-consuming when there are a lot of logs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, there isn't additional scan here. scanLinesOrGiveLong
is just a custom scan function replacing the default one. Comparing the default scan which is bufio.ScanLines
, scanLinesOrGiveLong
just a simple function wrapping bufio.ScanLines
with some extra checking to ensure the token returned wouldn't be too large to cause bufio.ErrTooLong
error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alexec Any objections?
scanner := bufio.NewScanner(stream) | ||
//give it more space for long line | ||
scanner.Buffer(make([]byte, startBufSize), maxTokenLength) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don’t really understand this PR. Is is based on any docs or blog post you could share please?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
scanLinesOrGiveLong
is to let scanner avoid bufio.ErrTooLong
error, which will cause scanner.Scan()
return false, thus break the loop early, leaving logs in the stream not scanned. This method is borrowed from here.
Actually, the method above alone is sufficient to solve the issue #9573 , but it will be nicer with the complementary of the following.
scanner.Buffer(make([]byte, startBufSize), maxTokenLength)
is to enlarge the scanner's inner buffer(the default size is 4096), and replace the MaxScanTokenSize(the default value is 64 * 1024) with a larger one. According to our observation, it is common that users print visual progress bar to stdout. Most of the visual progress bar use the '\r' character to return to the start point of the same line, as to give user-friendly visual effect on terminal. Thus, it forms a very long line for scanner, as scanner treats '\n' as the end of each line by default. if we don't set more larger MaxScanTokenSize, the progress bar will be chopped in the middle very often. it is not a big deal, but larger one will make the logs looks nicer in this case.
It is worthy to mention that this issue can be solved by another method:
instead of looping over stream with the help of scanner, we can do this manually, check each byte from the stream, and get the new line when seeing a new '\n'.
This method have a drawback, user may print a long line in a very long time, such as the progress-bar-like logs, then the user will wait very long time and then see a very long line log pop up abruptly.
Signed-off-by: pengfei.ji <[email protected]> Co-authored-by: pengfei.ji <[email protected]> Signed-off-by: juchao <[email protected]>
Fixes #9573
argo logs abort early caused by bufio.ErrTooLong