Conversation
There was a problem hiding this comment.
Pull request overview
Adds configurable retry grouping for MySQLi operations, enabling different retry policies per error type (e.g., reconnect vs. deadlock).
Changes:
- Introduces
RetryGroupandRetryCollectionabstractions to manage retry rules by status/error codes. - Updates
Mysqli::rawQuery()andMysqli::escape()to use the new retry collection and adds a retry hook (handleRetriedException()). - Replaces the old single
connectionRetriesmechanism and removes thesetConnectionRetries()public API.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/Driver/Mysqli/RetryGroup.php | New group primitive: maps status codes to a retry budget and tracks retry usage. |
| src/Driver/Mysqli/RetryCollection.php | New collection: manages multiple retry groups and decides whether an error can be retried. |
| src/Driver/Mysqli/Mysqli.php | Switches query/escape retry logic to the new collection and introduces a retry hook; removes old retry setter. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
pavog
left a comment
There was a problem hiding this comment.
In general, I find the term "retries" a bit confusing.
Because 1 retry means that it has already been tried twice. And 0 retries means: Tried once.
What do you think about changing from retries to attempts ?
| } | ||
|
|
||
| /** | ||
| * @param array $statusCodes |
| */ | ||
| public function matchesStatusCodes(array $statusCodes): bool | ||
| { | ||
| foreach ($statusCodes as $statusCode) { |
There was a problem hiding this comment.
We could also use
return array_any($statusCodes, fn($statusCode) => $this->matchesStatusCode($statusCode));here
| */ | ||
| protected function getRetryCollection(): RetryCollection | ||
| { | ||
| return new RetryCollection([ |
There was a problem hiding this comment.
Are you sure it's a good idea to set the retries to 1 and 3 by default?
The default behavior of PHP / Mysqli would be 1 try (-> 0 retries).
I'm concerned that people might overlook that this is implemented differently in this library.
No description provided.