Added keep-alive strategy #26
Conversation
…umberOfConnections * removed array $options from Credentials * Added ConnectionOptions, and extended class ConnectionPoolOptions. * Using ConnectionPoolOptions in unit tests. * SingleConnection add a periodic timer to the loop do a SELECT 1 when ConnectionOptions int $keepAliveIntervalSec has been set * On close of destruct the timer, if any, is removed from the loop
…::createConnected to $connection->connect(...)
|
@mmoreram Hi Marc, as you can see I implemented the keep alive strategy. Please review it. |
|
@mmoreram there is a breaking change. I removed the $connections from Credentials class since the "number of connections" is now in a different class ConnectionPoolOptions which extends the ConnectionOptions class. Seems a more logical place? I also removed the array $options from Credentials since that did not seem to be used, and we have a class ConnectionOptions now? The ConnectionOptions class has a int $keepAliveIntervalSec . Do you think we need to keep supporting the int $connections in the Credentials class for backwards compatibility? Of course, we can do that if you want. |
@mmoreram I will look into making it backwards compatible with a deprecations notice. |
|
@mmoreram ok, it should be backwards compatible now, but review it carefully |
| $this->keepAliveTimer = Loop::get()->addPeriodicTimer($options->getKeepAliveIntervalSec(), function() { | ||
| $qb = $this->createQueryBuilder(); | ||
| $this->query( | ||
| $qb |
There was a problem hiding this comment.
Are we sure that this format works properly in all adapters? Would make sense to add a ping() method in the interface with all implementations and tested.
There was a problem hiding this comment.
for all current adapters postgresql, sqlite and mysql SELECT 1 is working. I verified mysql in our own implementation, and used online tools for the other 2:
https://extendsclass.com/postgresql-online.html
https://sqliteonline.com/
SELECT 1 is probably the smallest possible SELECT statement there is , so I cannot imagine any sql database not supporting this.
If you still have second thoughts let me know, I will write the ping but it will be equal for all current adapters
| [], 10 | ||
| ), $mysqlPlatform); | ||
| 'test' | ||
| ), $mysqlPlatform, new ConnectionPoolOptions(10)); |
There was a problem hiding this comment.
Could we test this new feature somehow?
There was a problem hiding this comment.
we already use the fork in our MSP Challenge project. I will do a re-test and make a quick video capture to share it as "proof" that is works. I can show what happens using Grafana I think.
I think I will be able to show you early next week, so please wait for a while until I get back
Added keep-alive strategy as suggested by @mmoreram in #24