Code review comment for lp:~michael.nelson/launchpad/588288-log-parser-dont-read-entire-file-qa

Revision history for this message
Abel Deuring (adeuring) wrote :

Hi Michael,

your changes look good -- but I believe I found another issue in the "while next_line" loop (or I had not have yet enough coffee...):

If we get any exception in the second try/except block of parse_file(), parsed_bytes is set to the value for the start of the line causing the exception, and the loop is terminated via a "break".

So, when parse_file() is called the next time, the line causing the exception is read again. That's fine if the exception of the previous run was due to the line being truncated; in this case, it is fair to assume that a new run with more data will work better. But if the exception is caused for example by a bug in get_method_and_path() or geoip.getRecordByAddress(), we will have an "non-terminating loop", in the sense that sucessive calls of parse_file() will never get past the line causing the exception.

I'm approving this branch but I think it is worth to rethink the exception handling in the loop.

review: Approve (code)

« Back to merge proposal