fix: Negative collection_offset Causes Multiple Executions#18178
fix: Negative collection_offset Causes Multiple Executions#18178majiayu000 wants to merge 3 commits intoinfluxdata:masterfrom
Conversation
When using a negative collection_offset value, the AlignedTicker could calculate a duration less than or equal to zero, causing the timer to fire immediately and repeatedly. This resulted in multiple executions and "Previous collection has not completed" debug messages. The fix ensures that when the calculated duration (time to next aligned interval plus offset) is non-positive, one full interval is added to schedule for the next valid tick time. This check is performed before adding jitter to maintain proper timing behavior. Also updated the sleep function to handle negative durations by returning immediately instead of creating a timer with a negative value. Fixes influxdata#18175 Signed-off-by: majiayu000 <1835304752@qq.com>
|
Shouldn't the solution be to let Telegraf error out when a negative duration is configured anywhere? |
|
@majiayu000 adding to the comment of @Hipska, what is the reasoning for a negative offset? I'm asking because the effect is basically the same as doing |
|
Download PR build artifacts for linux_amd64.tar.gz, darwin_arm64.tar.gz, and windows_amd64.zip. 📦 Click here to get additional PR build artifactsArtifact URLs |
srebhan
left a comment
srebhan
left a comment
There was a problem hiding this comment.
Thanks @majiayu000 for the update! I think you should error out in cases where
- the offset is larger than
interval - the offset is negative and its absolute value is larger than
interval(i.e.offset + interval < 0) - the interval is negative
Given all that I'm not sure if we should support negative offsets at all... It seems to me that the semantics of a negative offset is somewhat ambiguous and chances are high that the user means something different from what we implement. So how about simply error out and suggest to use interval - offset instead?
| // Ensure the duration is positive before adding jitter. This can still | ||
| // happen with small intervals and clock adjustments. | ||
| if d <= 0 { | ||
| d += t.interval | ||
| } |
| if interval <= 0 || offset == 0 { | ||
| return offset | ||
| } |
There was a problem hiding this comment.
Please error out during configuration if the interval is negative! This is clearly a misconfiguration!
| if interval <= 0 || offset == 0 { | ||
| return offset | ||
| } | ||
| offset = offset % interval |
There was a problem hiding this comment.
Isn't this unexpected to the user? I rather prefer to error out during configuration if the offset is larger than the interval...
| if offset < 0 { | ||
| offset += interval | ||
| } |
There was a problem hiding this comment.
Please make sure that the result is positive during configuration and error out otherwise!
3 participants
Fixes #18175
Changes