Conversation
twiggler
force-pushed
the
fix/mssql-errorlog
branch
from
ca7a033 to
f93f855
Compare
twiggler
left a comment
twiggler
left a comment
There was a problem hiding this comment.
Can you add unit tests?
There are a lot of potential edge cases :)
| # If we have a buffer with a timestamp and | ||
| # our current line also has a timestamp, | ||
| # we should have a complete record in our buffer. | ||
| if previous_ts := RE_TIMESTAMP_PATTERN.match(buf): |
There was a problem hiding this comment.
This potentially causes memory exhaustion if the log file does not start with a timestamp.
(And no entries will get returned)
I think the logic I posted here does not suffer from that problem
|
|
||
| # For the last line | ||
| if buf: | ||
| if current_ts := RE_TIMESTAMP_PATTERN.match(line): |
There was a problem hiding this comment.
This only works if the last line is timestamped. If it is text, it will fail and the last record will be dropped.
The solution is to use current_ts
|
@twiggler I took a second look at your logic but I could not get it to work. Had issues with messages that had newlines and tabs in it. I added a check to see if a logfile starts with a timestamp and skip it if it does not. Would be very weird for this log format. Added some unit tests and changed the test file a little bit to make sure that the last log entry contains a newline which we can test on. Lmk what u think. |
Hi, can you share (parts of, and/or redacted) the logfile that leads to problems? |
|
Try the test data file |
2 participants
closes #1331