fix: seat offset spanvec correctly after truncation - #644
Conversation
ae1f319 to
0569f45
Compare
5b4977a to
ef026df
Compare
| let offset = inner.offset; | ||
|
|
||
| // truncate offsets to new file length | ||
| inner.offsets.truncate(offset); |
There was a problem hiding this comment.
Hmm, we may need to gate this behind Lookback::None, right? Otherwise the common case of truncate, write line, agent gets inotify, agent reads file would miss the line written immediately after the truncation as we've no way to determine the length the file was actually truncated to
There was a problem hiding this comment.
It's not perfect, but it's significantly better than what it's currently doing now, i.e partial truncations are causing all lines to be re-ingested from the file start which is in turn causing odd behavior (possibly corrupted logs).
I don't believe there's an ideal solution here due to limitations with how inotify works and the inherent delay between events and state updates.
We could potentially stat the size of a file as soon as an event is registered and use that metadata in our decision making process - this would help minimize these issues but not remove them completely.
Alternatively we could tie the truncation logic to follow the configured lookback logic or add a new config option for different truncation strategies. Right now the truncation logic always uses a strategy which is effectively the same as lookback:smallfiles
There was a problem hiding this comment.
Right now the truncation logic always uses a strategy which is effectively the same as lookback:smallfiles
Yeh, that's always been a weird shortcut. I think if we're going to make this change we need to gate it behind a new piece of config, I don't know if it should be the default or not, but just enabling it will result in the standard copy-truncate losing any lines that skip it past the 8192 bytes heuristic. At least if we make it configurable/opt in then users can be aware of the trade off
There was a problem hiding this comment.
@c-nixon I've made the truncation strategy configurable and is set up similar to lookback. I've kept the default setting of smallfiles to be consistent with previous intended behavior, but it kind of sounded like you wanted to default to start - I still need to implement full testing of each truncate strategy, but wanted to get your thoughts on what's here now!
4275835 to
71eb57b
Compare
22572af to
b0973f1
Compare
When truncating large files, the offsets SpanVec was incorrectly seeking to the beginning of the file instead of the new offset (the end of the file). Adds a new method to truncate `SpanVec` and utilize it during file truncation. Additionally `utils/stress_test` since it's not used, the previous maintainer is not working on it anymore, and it's breaking our CI/CD tooling. Ref: LOG-21703 Signed-off-by: Jacob Hull <jacob@planethull.com>
b0973f1 to
b35ae8c
Compare
| .expect("Failed to assign tailer process to job."); | ||
|
|
||
| let tailer_stdout = tailer_process.stdout.take().ok_or_else(|| { | ||
| #[allow(clippy::io_other_error)] |
There was a problem hiding this comment.
World's most minor nit: We can update the call to the new std::io::Error::other(e) which rather than std::io::Error::new which will not trigger the lint
c-nixon
left a comment
c-nixon
left a comment
There was a problem hiding this comment.
Yeh, this is looking great so far, maybe it's worth splitting the removal of the stress test into a separate PR? The truncation logic is all good, pity we can't just newtype Lookback, but we'd still end up having to reimplement it's Display etc, so no real benefit
2 participants
When truncating large files, the offsets SpanVec was incorrectly seeking to the beginning of the file instead of the new offset (the end of the file). Adds a new method to truncate
SpanVecand utilize it during file truncation.Additionally
utils/stress_testsince it's not used, the previous maintainer is not working on it anymore, and it's breaking our CI/CD tooling.Ref: LOG-21703