Add support for exponential backoff on retry #1986
Conversation
There was a problem hiding this comment.
This looks quite good. Let me know what you think of the modifications.
Also, it may be worth prefixing any visible functions with redis_ given that C uses a global namespace. I didn't get any symbol conflicts when building the whole PHP 8 source tree but it's usually better to be safe about it.
Example of what I mean by renaming:
-struct Backoff {
+struct RedisBackoff {
int algorithm; /* index of algorithm function, returns backoff in microseconds*/
zend_long base; /* base backoff in microseconds */
zend_long cap; /* max backoff in microseconds */
@@ -10,17 +10,17 @@ struct Backoff {
};
/* }}} */
-zend_long default_backoff(struct Backoff *self, int retry_index);
+zend_long redis_default_backoff(struct RedisBackoff *self, int retry_index);
/* etc... */
library.c
Outdated
| if (redis_sock->retry_interval) { | ||
| // Random factor to avoid having several (or many) concurrent connections trying to reconnect at the same time | ||
| long retry_interval = (count ? redis_sock->retry_interval : (php_rand() % redis_sock->retry_interval)); | ||
| zend_long retry_interval = backoff_compute(&redis_sock->backoff, retry_index); | ||
| usleep(retry_interval); | ||
| } |
There was a problem hiding this comment.
Maybe we can remove the if (retry_interval) { ... } and just compute the backoff with backoff_compute. Simpler to follow I think.
/* Sleep based on our backoff algorithm */
zend_long delay = backoff_compute(&redis_sock->backoff, retry_index);
if (delay != 0)
usleep(delay);From my initial testing this works as expected (e.g. returns 0 if retry_interval is 0)
backoff.c
Outdated
| return MIN(self->cap, backoff); | ||
| } | ||
|
|
||
| zend_long exponential_backoff(struct Backoff *self, int retry_index) { |
There was a problem hiding this comment.
Do we really need exponential backoff if it is always worse than jitter backoff? Is there any use cases why users may choose it?
There was a problem hiding this comment.
I'm honestly not sure. Maybe we could reorder the array of options to put the best overall algorithms first?
There was a problem hiding this comment.
Or maybe we could remove useless algorithms? 😄
There was a problem hiding this comment.
I think you make a good point.
@nbraun-amazon thoughts?
There was a problem hiding this comment.
I'd say leave the current option first, then reorder. I did it in this new revision.
There was a problem hiding this comment.
@yatsukhnenko I don't mind leaving them in (as non-defaults and at the end of the list). I can see a use for testing since the non-jitter algorithms are deterministic.
|
Hey @michael-grunder, thanks for your feedback :) I submitted a new version. I prefixed with either |
|
Once we agree, I'll also update the documentation. Thanks! |
redis_commands.c
Outdated
| case REDIS_OPT_BACKOFF_ALGORITHM: | ||
| val_long = zval_get_long(val); | ||
| if(val_long >= 0 && | ||
| val_long < BACKOFF_ALGORITHMS) { |
There was a problem hiding this comment.
This was changed to REDIS_BACKOFF_ALGORITHM
backoff.c
Outdated
|
|
||
| #include "backoff.h" | ||
|
|
||
| zend_ulong random_range(zend_ulong min, zend_ulong max) { |
There was a problem hiding this comment.
Might as well declare this static.
|
LGTM, if you make those two tiny changes I'll get this merged. (I fat-fingered the "close pull request" on accident) 😄 |
|
hey @michael-grunder, I made the changes accordingly. Please note that the documentation has not been updated yet. Let's discuss it. |
|
@nbraun-amazon could you also rebase your PR onto |
|
@yatsukhnenko done |
|
Build is failing because of missing |
9a3d50c to
781e526
Compare
|
@michael-grunder I pushed a new revision which should fix the 7.0 build. I guess you can approve the workflow so we can double-check it builds correctly. Thanks! |
|
@yatsukhnenko If you're OK with this I'll get it merged tomorrow. |
|
All checks passed :) |
|
Merged, thanks! |
Add documentation for backoff algorithms Related to #1986
No description provided.