Introduce Backoff implementation with exponential strategy
#50
Conversation
yhabteab
force-pushed
the
exponential-backoff
branch
from
b8d82b5 to
39be826
Compare
yhabteab
force-pushed
the
exponential-backoff
branch
2 times, most recently
from
e66eec5 to
5576ae0
Compare
| protected $retries; | ||
|
|
||
| /** @var ?int The previous used retry wait time */ | ||
| protected $previousWaitTime; |
There was a problem hiding this comment.
I would not maintain state and just do $min << $attempt as in our go code.
There was a problem hiding this comment.
He didn't do that on my request.
There was a problem hiding this comment.
Unnecessarily complex and difficult to understand for no apparent advantage
There was a problem hiding this comment.
php > $min = 100;
php > var_dump($min*2, $min << 1);
php shell code:1:
int(200)
php shell code:1:
int(200)
php > var_dump($min*2*2, $min << 2);
php shell code:1:
int(400)
php shell code:1:
int(400)
php > var_dump($min*2*2*2, $min << 3);
php shell code:1:
int(800)
php shell code:1:
int(800)
...There was a problem hiding this comment.
Unnecessarily complex and difficult to understand for no apparent advantage
You can't be serious. If this is too complex (2 ** $attempt * $min), I wonder how we can build software.
The obvious advantage is that no state is introduced without reason, which is then also changed in a getter.
The ipl should be an example of good coding practice. The current implementation is not.
The seemingly not so obvious advantage is that this does not fall apart when running asynchronously. Sure, PHP isn't ready to support everything concurrently yet, but PHP isn't the only programming language and there are fibers, async I/O (ReactPHP and the like) that already make this possible. When state like this one is introduced, concurrency should also be thought of. Even in PHP.
Also, getWaitTime() claims to return a proper wait time in the first line and then explains in the description that it actually does not. Calling getWaitTime(n) yields wrong results without having called getWaitTime(0..n-1) first.
Plus, when fixed, the actual backoff implementation would be properly testable.
yhabteab
force-pushed
the
exponential-backoff
branch
2 times, most recently
from
7ec528f to
4230092
Compare
yhabteab
force-pushed
the
exponential-backoff
branch
from
4230092 to
81f3d67
Compare
| protected $retries; | ||
|
|
||
| /** @var ?int The previous used retry wait time */ | ||
| protected $previousWaitTime; |
There was a problem hiding this comment.
Unnecessarily complex and difficult to understand for no apparent advantage
You can't be serious. If this is too complex (2 ** $attempt * $min), I wonder how we can build software.
The obvious advantage is that no state is introduced without reason, which is then also changed in a getter.
The ipl should be an example of good coding practice. The current implementation is not.
The seemingly not so obvious advantage is that this does not fall apart when running asynchronously. Sure, PHP isn't ready to support everything concurrently yet, but PHP isn't the only programming language and there are fibers, async I/O (ReactPHP and the like) that already make this possible. When state like this one is introduced, concurrency should also be thought of. Even in PHP.
Also, getWaitTime() claims to return a proper wait time in the first line and then explains in the description that it actually does not. Calling getWaitTime(n) yields wrong results without having called getWaitTime(0..n-1) first.
Plus, when fixed, the actual backoff implementation would be properly testable.
| public function setMin(int $min): self | ||
| { | ||
| if ($min <= 0) { | ||
| $min = 100; // Default minimum wait time 100 ms |
There was a problem hiding this comment.
Please introduce consts for the defaults.
| $this->assertNotSame(10, $backoff->setRetries(5)->getRetries()); | ||
| } | ||
|
|
||
| public function testGetWaitTime() |
There was a problem hiding this comment.
Should also test the waiting time for attempt n, where 0..n-1 has not yet been called.
| try { | ||
| return $callback($previousErr); | ||
| } catch (Exception $err) { | ||
| if ($attempt >= $this->getRetries() || $err === $previousErr) { |
There was a problem hiding this comment.
Why is the logic for deciding whether something is retryable not part of this code, but must be executed in each individual callback? With the current implementation, the decision whether something is retryable is made after sleeping (the next time the callback is called), and the first attempt cannot stop the retryable logic (please add a test). And requires duplicate code in each individual callback.
No description provided.