-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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(file source): Handle legacy fingerprint checksums from < v0.14.0 #8225
Conversation
Fixes: #8182 Signed-off-by: Jesse Szwedko <jesse@szwedko.me>
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.
Looks reasonable, and I appreciate moving more logic into the maybe upgrade function, though I do have a question about the buffer parameter.
path: &Path, | ||
fng: FileFingerprint, | ||
fingerprinter: &Fingerprinter, | ||
fingerprint_buffer: &mut Vec<u8>, |
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 buffer does not appear to be used by any of the callers after calling the function. Could it be created within this function to avoid the additional parameter?
In fact, it doesn't look like the contents are used within this function either. I don't think this function is on the hot path (startup only) so reusing the buffer shouldn't be critical to performance. Am I missing something?
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.
Agreed, I was following the pattern of the other functions, but think we can just create the buffer in here since it is only called once at start-up.
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, I realized the default buffer size 1 MB so I may replace this. I can see why we'd want to reuse them in-case there are a lot of files to avoid allocating 1 MB each time.
Signed-off-by: Jesse Szwedko <jesse@szwedko.me>
Signed-off-by: Jesse Szwedko <jesse@szwedko.me>
Signed-off-by: Jesse Szwedko <jesse@szwedko.me>
…8225) * fix(file source): Handle legacy fingerprint checksums from < v0.14.0 Adds logic to convert from checkpoints written by Vector before version 0.14.0 to 0.14.0 checkpoints by seeing if any checkpoints, that don't match an existing file, match if the old checkpoint strategy is used. Also adds alias for `first_line_checksum` strategy which would have broken checkpoints if released. Signed-off-by: Jesse Szwedko <jesse@szwedko.me> Signed-off-by: Luc Perkins <luc.perkins@datadoghq.com>
Adds logic to convert from checkpoints written by Vector before version 0.14.0 to 0.14.0 checkpoints by seeing if any checkpoints, that don't match an existing file, match if the old checkpoint strategy is used.
Also adds alias for
first_line_checksum
strategy which would have broken checkpoints if released.Ideally I'd like to include this in 0.15.0 to avoid needing to release a 0.15.1 with it.
This will be released as a 0.13.1.
Fixes: #8182
TODO:
test_checkpointer_fingerprint_upgrades_legacy_checksum
is failing. It seems to calculate a different checksum than I get when running 0.13.1 directly despite the upgrade working when I run this branch using 0.13.1 checkpoints.Signed-off-by: Jesse Szwedko jesse@szwedko.me