-
Notifications
You must be signed in to change notification settings - Fork 241
Introduce retry metadata to batch struct #1970
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
base: enable-multithreaded-logging-by-default
Are you sure you want to change the base?
Conversation
| s.logger.Errorf("All %v retries to %v/%v failed for PutLogEvents, request dropped.", retryCountShort+retryCountLong-1, batch.Group, batch.Stream) | ||
| // Check if retry would exceed max duration | ||
| totalRetries := batch.retryCountShort + batch.retryCountLong - 1 | ||
| if batch.isExpired(s.RetryDuration()) || batch.nextRetryTime.After(batch.startTime.Add(s.RetryDuration())) { |
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.
Wont the 2nd condition in this line always be triggered before isExpired returns true?
Also, can the second part be moved into isExpired()? Would the following achieve the same?
maxWaitTime := b.startTime.Add(maxRetryDuration)
b.nextRetryTime.After(maxWaitTime) || time.Now().After(maxWaitTime)
If this is equivalent to your logic, then even here we can see that the first part will always be true before the second part can be true. Since nextRetryTime in n-1 iteration will be more than time.Now() in the nth iteration.
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.
I guess the time.Now() check in isExpired() is useful for the subsequent PRs when the RetryHeapProcessor comes into play?
In that case, should the condition here just be:
if batch.nextRetryTime.After(batch.startTime.Add(s.RetryDuration()))
| } | ||
|
|
||
| // isExpired checks if the batch has exceeded the maximum retry duration. | ||
| func (b *logEventBatch) isExpired(maxRetryDuration time.Duration) bool { |
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.
nit: Can we just store a field called expireAfter on the logBatch which holds when that batch will expire? You can then just set that when the batch is created based on the start time and maxDuration. That way you dont need to pass it in each time here and more importantly, you dont need to pass it to the RetryHeapProcessor.
You also dont need to worry if diff batches have diff max durations for some reason down the line etc - as the RetryHeapProcessor is common.
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.
Yeah it was an alternate option, i saw them as equvalent. The reason I kept this is because the maxRetryDuration seems to be a parameter in the existing code, so I wasnt clear if this can somehow change, and kept the same. Will consider revising in a follow up
Description of the issue
Track putlogevents retry metadata as part of the batch struct to persist retry data.
Description of changes
Changes include structural changes, no functional changes. This is in preparation for subsequent changes to the PLE retry processing logic.
License
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.
Tests
Unit testing passes
Requirements
Before commiting your code, please do the following steps.
make fmtandmake fmt-shmake lintIntegration Tests
To run integration tests against this PR, add the
ready for testinglabel.