-
Notifications
You must be signed in to change notification settings - Fork 25
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: Adjust big segments logic to prevent duplicate polls. #225
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,6 +22,7 @@ internal class BigSegmentStoreWrapper : IDisposable | |
private readonly CancellationTokenSource _pollCanceller; | ||
private readonly ReaderWriterLockSlim _lock = new ReaderWriterLockSlim(); | ||
private readonly Logger _logger; | ||
private readonly Task<BigSegmentStoreStatus> _initialPoll; | ||
|
||
private BigSegmentStoreStatus? _lastStatus; | ||
|
||
|
@@ -43,8 +44,9 @@ Logger logger | |
_taskExecutor = taskExecutor; | ||
_logger = logger; | ||
|
||
_initialPoll = Task.Run(PollStoreAndUpdateStatusAsync); | ||
_pollCanceller = taskExecutor.StartRepeatingTask( | ||
TimeSpan.Zero, | ||
config.StatusPollInterval, | ||
config.StatusPollInterval, | ||
PollStoreAndUpdateStatusAsync | ||
); | ||
|
@@ -119,11 +121,7 @@ internal BigSegmentStoreStatus GetStatus() | |
{ | ||
_lock.ExitReadLock(); | ||
} | ||
if (ret.HasValue) | ||
{ | ||
return ret.Value; | ||
} | ||
return AsyncUtils.WaitSafely(() => PollStoreAndUpdateStatusAsync()); | ||
return ret ?? _initialPoll.GetAwaiter().GetResult(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If there is no value, then that first poll has not completed. Wait for it to complete. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it possible that the initial task is still executing by the time the first repeating task runs? I suppose it may be extremely unlikely given the real world values of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It theoretically is possible. I did consider if there was a way to use the most recent task, but the complexity didn't seem worth it. If at any point a repeating task does complete, then we would ideally hit the first code path subsequently. |
||
} | ||
|
||
private async Task<BigSegmentStoreStatus> PollStoreAndUpdateStatusAsync() | ||
|
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.
Manually run the first task instead of using the repeating task.