in_tail: position_file: Define path based equalities on TargetInfo #3391
Conversation
cosmo0920
force-pushed
the
define-target-info-equalities
branch
from
883eaa7
to
2c9b31f
Compare
|
Typo? -In the previous Fluentd v1.11.x implementation uses patch based tailing keys.
+In the previous Fluentd v1.11.x implementation uses path based tailing keys. |
In the previous Fluentd v1.11.x implementation uses path based tailing keys. We wouldn't rewrite tailing mechanism entirely. And also we're implicitly assuming path based equality for TargetInfo in tailing logic. We should handle TargetInfo with path based equality instead of path and inode based equality. Signed-off-by: Hiroshi Hatake <hatake@calyptia.com>
cosmo0920
force-pushed
the
define-target-info-equalities
branch
from
2c9b31f
to
e516268
Compare
Yep. This is typo. I'd fixed it. |
|
Hmm, we should release v1.12.4 with this patch before releasing v1.13? |
|
I'll create v1.12 branch. |
Yes, please. |
|
I'll release v1.12.4 in a few days. |
| end | ||
|
|
||
| def hash | ||
| self.path.hash |
There was a problem hiding this comment.
I'm not sure follow_inodes feature but does it work well also on @follow_inodes?
I think that keys of @tails should be inode on @follow_inodes.
There was a problem hiding this comment.
Or paths are updated by update_watcher on rotation so paths are never duplicate?
There was a problem hiding this comment.
I'll take a look on this.
There was a problem hiding this comment.
I'm still checking it too.
Probably above my concern is wrong, it should be path maybe.
Otherwise update_watcher can't discard old TailWather from @tails.
There was a problem hiding this comment.
Memo:
Instead of overriding them, I thought using target_info.path as a key is more straightforward and understandable: @tails[target_info.path]
But it's also wrong.
TargetInfo is referred by each_keys so keys have to be TargetInfo.
There was a problem hiding this comment.
Hmm, probably it's O.K. as a first aid but I think more refactoring is needed to make easy to understand in the future 🤔
There was a problem hiding this comment.
Instead of overriding them, I thought using
target_info.pathas a key is more straightforward and understandable:@tails[target_info.path]But it's also wrong.
TargetInfois referred byeach_keysso keys have to beTargetInfo.
Yes! I'm also considering to use it to be made easier to understand, but overriding equality on TargetInfo is one of the simplest way to dispose discarded old TailWatcher correctly. (This strategy doesn't request more refactoring.)
| t1.eql? t2 | ||
| end | ||
| end | ||
|
|
There was a problem hiding this comment.
To emphasize the difference of eql? and equal? behavior, how about adding one more test case using assert_not_same t1, t2?
Signed-off-by: Hiroshi Hatake <hatake@calyptia.com>
…sub_test_case Signed-off-by: Hiroshi Hatake <hatake@calyptia.com>
|
I've created a pull request for v1.12 and merged already: #3393 |
Signed-off-by: Hiroshi Hatake hatake@calyptia.com
Which issue(s) this PR fixes:
Related to #3365
What this PR does / why we need it:
In the previous Fluentd v1.11.x implementation uses path based tailing
keys.
We wouldn't rewrite tailing mechanism entirely.
And also we're implicitly assuming path based equality for TargetInfo in tailing logic.
We should handle TargetInfo with path based equality instead of path and
inode based equality.
Docs Changes:
No needed.
Release Note:
Same as title.