Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added connection pool #24

Merged
merged 2 commits into from
Apr 14, 2022
Merged

Added connection pool #24

merged 2 commits into from
Apr 14, 2022

Conversation

mmoreram
Copy link
Member

No description provided.

@mmoreram mmoreram force-pushed the feature/added-connection-pool branch from a8e7c13 to 9e8f29f Compare December 10, 2021 15:58
@mmoreram mmoreram force-pushed the feature/added-connection-pool branch from 9e8f29f to eaa905f Compare December 10, 2021 16:01
@marinhekman
Copy link
Contributor

@mmoreram Hi Marc, we are using driftphp dbal together with RacthetPHP for the project www.mspchallenge.info. I think a connection pool would be a great addition . So what is the status of this PR ?
If you want I can test this branch with our software to see if there are still any issues

@mmoreram
Copy link
Member Author

Hello @marinhekman

Thanks for the comment and for using the project. I'm glad seeing that the project is useful :)
Sorry for leaving this PR on standby. Of course. If you can test this branch to see what's the behavior on "real" projects, I will do the same with mine (@apisearch-io), and if everything runs as expected, then, I will merge and tag the package.

@marinhekman
Copy link
Contributor

@mmoreram perfect. I will let you know within a couple of weeks

@marinhekman
Copy link
Contributor

marinhekman commented Apr 8, 2022

@mmoreram I succesfullly tested with 40 connections. Basically replaced
Connection::createConnected with ConnectionPool::createConnected and added the number of connections to the Credentials constructor call. Queries just work. I can see the connection in the "process list".

Question: how would you handle a mysql idle timeout?
Since I am running a websocket server, and without any activity for like 8 hours, the connection would break.
With a single connection I could just do a dummy query to "prevent" the time-out like this:

        // do a dummy SELECT 1 query every 4 hours to prevent the "wait_timeout" of mysql (Default is 8 hours).
        //  if the wait timeout would go off, the database connection will be broken, and the error
        //  "2006 MySQL server has gone away" will appear.
        $loop->addPeriodicTimer(14400, function () {
            $qb = $this->connection->createQueryBuilder();
            $this->connection->query(
                $qb->select('1')
            );
        }

Of course with a pool I would have to traverse each one these pool connections...not the best solution, and I not even sure I can traverse each connection now.

Maybe there a way to just handle the "2006 MySQL server has gone away" exception and do a re-connection ? I can probably handle the exception in the rejection of the promise, but how do I tell a DriftPHP dbal connection to do a re-connection? Maybe calling connect() would work.. I will try that

@marinhekman
Copy link
Contributor

@mmoreram it would be handy to have a way to iterate over the connections in the pool.

@marinhekman
Copy link
Contributor

I did this for now: marinhekman@c4f1a40

@mmoreram
Copy link
Member Author

Hello!

Thanks for your work :) I really appreciate it.
I'm thinking about this new ping strategy to keep connections up and running, and to be honest, it's perfect. I think that I'm having some similar issues in some of my projects, and that would solve them as well.

I'm thinking then.
Why don't we add this logic inside the connection pool?
Seems a good move to return all workers, but we could explore this, and even add in the library the keep-alive strategy.

@mmoreram
Copy link
Member Author

@marinhekman I'm mergin both your PR and mine, and will add a minor release for you to start using it on prod.

If you want to explore the addition of this keep alive inside the library, that would be awesome.
My 5 cents.

  • Add a keep_alive_each or similar parameter inside the new connection creation method
  • If this is defined, then add the loop periodic timer
  • If the connection is closed, detach this timer
  • If we have a connection pool, do the same but with all timers

@mmoreram mmoreram merged commit fd55f3a into master Apr 14, 2022
@mmoreram mmoreram deleted the feature/added-connection-pool branch April 14, 2022 16:36
@marinhekman
Copy link
Contributor

@mmoreram thanks . I will explore the addition of keep_alive_each suggestion in my fork and let you know.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants