Feature/usage sharing backoff #218
Conversation
…re namespace for improved structure and reusability. Updated namespace references across affected files.
…rove fail-handling during data queueing.
…st recovery logic.
| public ExponentialBackoffRecoveryStrategy( | ||
| double initialDelaySeconds = 2.0, | ||
| double maxDelaySeconds = 300.0, | ||
| double multiplier = 2.0) |
There was a problem hiding this comment.
Default values should be exposed as constants rather than exist in signature, see
| { | ||
| lock (_lock) | ||
| { | ||
| _consecutiveFailures++; |
There was a problem hiding this comment.
Unconditional increment can cause unexpected skips of exponential stages in multi-threaded environment.
[00:10.50] (4x threads) MayTryNow returns true
[00:11.03] (thread A) RecordFailure -- stage 1 (2s recovery)
[00:11.05] (thread B) RecordFailure -- stage 2 (4s recovery)
[00:11.06] (thread C) RecordFailure -- stage 3 (8s recovery)
[00:11.08] (thread D) RecordFailure -- stage 4 (16s recovery)
^ sending locked for 16 seconds almost immediately
There was a problem hiding this comment.
advised to use interlock increment operations rather than raw locks, whenever we just need to increment an integer or similar
| /// Third failure: wait initialDelay * multiplier^2 seconds | ||
| /// And so on, up to maxDelaySeconds. | ||
| /// </summary> | ||
| public class ExponentialBackoffRecoveryStrategy : IRecoveryStrategy |
There was a problem hiding this comment.
Consider amending CloudRequestEngineBuilder.NewEngine with capacity to instantiate this strategy.
Potentially, move the logic into a shared dedicated factory/method.
| } | ||
|
|
||
| _httpClient = httpClient; | ||
| _recoveryStrategy = new ExponentialBackoffRecoveryStrategy(); |
There was a problem hiding this comment.
Parametrization should be moved into the builder, see
| /// <summary> | ||
| /// Recovery strategy for handling failed attempts to add data to the queue. | ||
| /// </summary> | ||
| private readonly ExponentialBackoffRecoveryStrategy _recoveryStrategy; |
There was a problem hiding this comment.
Flow element should probably use the fail handler instead of strategy, see
This would allow exposing consistent options in JSON configuration.
| // Check if we're still in recovery period | ||
| if (!_recoveryStrategy.MayTryNow(out CachedException cachedException)) | ||
| { | ||
| return; |
There was a problem hiding this comment.
Since this element is going to silently ignore the unavailability,
the design of
may no longer be appropriate (as we shouldn't rely on exceptions for non-critical paths).
( * If we were to use it in this element -- see the comment above )
At the least, we should consider making the thrown exception not directly tied to Pipeline as a whole.
| using FiftyOne.Pipeline.CloudRequestEngine.FailHandling.ExceptionCaching; | ||
| using FiftyOne.Pipeline.CloudRequestEngine.FailHandling.Recovery; | ||
| using FiftyOne.Pipeline.Core.FailHandling.ExceptionCaching; | ||
| using FiftyOne.Pipeline.Core.FailHandling.Recovery; |
There was a problem hiding this comment.
Tests should be amended to account for the new ExponentialBackoffRecoveryStrategy
…pdate constructor to use them.
…ge flow elements for enhanced fail-handling and recovery logic.
…coveryStrategy for enhanced fail-handling logic. Updated recovery strategy flow in ShareUsage elements and added IsAvailable API for non-critical operations.
…tion and configuration.
No description provided.