-
Notifications
You must be signed in to change notification settings - Fork 1.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
in_tail: Ensure to detach correct watcher on rotation with follow_inodes #4208
in_tail: Ensure to detach correct watcher on rotation with follow_inodes #4208
Conversation
The scenario of this problem (basically from #4185 (comment)) When
If
|
19fa95d
to
8ec5545
Compare
8ec5545
to
684632b
Compare
Hi @daipom Thank you for the disuccsing regarding #3614. @masaki-hatada and Me have checked and tested heavily last week. |
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 good for me.
@kattz-kawa @masaki-hatada Thanks for your review! |
@ashie Thanks for your review! I'll check it. |
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.
LGTM (above my comments are nitpicking)
If `refresh_watchers` run before `update_watcher`, the old implementation of `update_watcher` detach wrongly the new TailWatcher which is added in `refresh_watcher`. This causes the problem of stopping tailing log and handle leak. The test case `test_updateTW_after_refreshTW` reproduces this problem. This fix solves it. There are another BUG about unwatching. I adjusted some expected values of the tests for this BUG. When `refresh_watcher` find the rotated old file AFTER unwatching it (`rotate_wait`), then the logs will be collected in duplicate. If `refresh_watcher` find it BEFORE unwatching it (`rotate_wait`), this problem doesn't occur because the position entry is still alive. We need to fix this and fix the adjusted expected values. This fix is based on the content and discussion of the following issue and PR: * fluent#3614 * fluent#4185 * fluent#4191 Signed-off-by: Daijiro Fukuda <[email protected]> Co-authored-by: Katuya Kawakami <[email protected]> Co-authored-by: Masaki Hatada <[email protected]> Co-authored-by: Gary Zhu <[email protected]> Co-authored-by: Takuro Ashie <[email protected]>
684632b
to
1a86721
Compare
Thanks to all contributors who help investigating this issue! |
TODO:
|
Thanks to all contributors who have given me much insight, especially @kattz-kawa, @masaki-hatada, and @garyzjq! |
Which issue(s) this PR fixes:
What this PR does / why we need it:
If
refresh_watchers
run beforeupdate_watcher
, the old implementation ofupdate_watcher
detach wrongly the new TailWatcher which is added inrefresh_watcher
. This causes the problem of stopping tailing log and handle leak.The test case
test_updateTW_after_refreshTW
reproduces this problem.This PR solves it.
NOTE: There is another BUG about unwatching. (I adjusted some expected values of the tests for this BUG.)
When
refresh_watcher
find the rotated old file AFTER unwatching it (rotate_wait
), then the logs will be collected in duplicate.If
refresh_watcher
find it BEFORE unwatching it (rotate_wait
), this problem doesn't occur because the position entry is still alive.We need to fix this and fix the adjusted expected values.
Docs Changes:
Not needed.
Release Note:
Same as the title.
OTHERS:
This fix is based on the content and discussion of the following issue and PR:
Thanks to those who have given me much insight, especially @kattz-kawa, @masaki-hatada, and @garyzjq!