-
Notifications
You must be signed in to change notification settings - Fork 18.6k
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 awslogs driver repeating last event - #34292 #34296
Fix awslogs driver repeating last event - #34292 #34296
Conversation
Signed-off-by: Justin Menga <justin.menga@gmail.com>
cc @samuelkarp |
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. Thanks for fixing this!
@@ -641,14 +641,28 @@ func TestCollectBatchMultilinePatternMaxEventAge(t *testing.T) { | |||
}) | |||
|
|||
// Fire ticker batchPublishFrequency seconds later | |||
ticks <- time.Now().Add(batchPublishFrequency * time.Second) | |||
ticks <- time.Now().Add(batchPublishFrequency + time.Second) |
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.
Is this correct? The comment leads me to believe it should be "* time.Second";
// Fire ticker batchPublishFrequency seconds later
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.
batchPublishFrequency
is already a time.Duration
(see line 40 in cloudwatchlogs.go), so the existing code was an error.
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.
ah, makes sense (reading on my phone)
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
ping @cpuguy83 PTAL
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. Thanks for the fix!
All Jenkins tests passed. Merging. |
@yongtang @thaJeztah Would this be cherry-picked into Docker 17.06.1-ce? |
I opened an issue internally to consider backporting it yes. Not sure yet if it makes 17.06.1, otherwise it could be included in a .2 release. |
Signed-off-by: Justin Menga justin.menga@gmail.com
- What I did
Fixes #34292
- How I did it
Ensure event buffer used for storing log events is properly flushed
- How to verify it
The modifications to the test case in this PR fail with the old code and pass with the new code.
See #34292 for details on how to replicate the issue (I have verified this is no longer an issue with this PR)
- Description for the changelog
Fix awslogs driver repeating last event
- A picture of a cute animal (not mandatory but encouraged)