add backend-agnostic connection check

[guillomovitch]

Summary

Each minion class backend uses a different attribute for its database connection object ('pg' for postgres backend, 'mysql' for mysql backend, etc). This makes low-level operations implementation-dependant.

Motivation

The change allows to send regular traffic to the backend, in order to avoid the database connection being closed for timeout, in a backend-agnostic way.

References

See discussion #2229.

[kraih]

I don't like that this requires changes in every single backend. Based on that fact i'm voting 👎 .

[jberger]

Could it also implement can_ping with a default implementation of return 0? Then backends can choose to implement or not. Or heck the default implementation of ping could just assume the worst case which is no different than now.

[kraih]

The whole idea of having a ping method in a job queue API seems wrong to me.

[kraih]

And it's not like ping is a huge sophisticated feature folks will get a lot of value out of. It's just a wrapper for a SELECT 1 query.

[guillomovitch]

The default minion setup, at least when used with minion plugin, is to establish a single persistent network connection with the backend, as long a the application is running. Ensuring this connection stays functional is mandatory when intermediate network equipments automatically cut off idle connections. So, the actual need is just to make easier to implement a keepalive mechanism to match this strategy.

Actually, small differences between backends (each one use a different property to store its database object) require the application to handle each backend differently, ie:

Mojo::IOLoop->recurring($delay => sub ($loop) {
    my $backend = $app->minion->backend;
    my $class = ref($backend);
    if ($class eq 'Minion::Backend::mysql') {
        $backend->mysql->db->ping;
    } elseif ($class eq 'Minion::Backend::Pg') {
        $backend->pg->db->ping;
    } else {
        ....
    }
});

With this small abstraction layer added to minion, this would turn to:

Mojo::IOLoop->recurring($delay => sub ($loop) {
    $app->minion->backend->ping;
});

This keepalive mechanism would also be a useful addition to Minion::Backend, but still requires this first part.

@jberger I may indeed add a can_ping method, but wouldn't just removing the stub implementation in Minion::Backend achieve the same result, using Universal::can, ie:

use Universal::can;
if ($backend->can('ping')) {
  // has backend-agnostic way of sending keepalive packets
} else {
  // has backend-specific way of sending keepalive packets
}

[kraih]

I get what you want to ultimately achieve, but the implementation makes no sense in a job queue API. Just look at the method description Check backend connection.. Ok, so it checks the connection... but then what? Why would i ever want to check if the connection is active? What do i as a user do if it isn't? It just doesn't make sense there.

[guillomovitch]

I just re-used the name and description of ping method in Mojo::Pg::Database. I can change the description to something as: "send unspecified trafic to the backend, to avoid the connexion turning idle", but that would just be describing one usage, even if probably the only one.

Alternatives solutions:

• instead of proving a shortcut to one specific method of the backend database object, provide a more generic db one, to expose this object for whatever usage
• do not expose this ping method (just renaming it to _ping, and not documenting it, for instance), but implement a keepalive($delay) method in Minion::Backend class that would rely on this private method to achieve the intended result

The first solution is basically making desencapsulation easier, the second one seems a better idea, but I'm unconfortable with the idea of private methods implemented in a child class, and used from a parent class.

[guillomovitch]

Last commit provides better documentation, in order to adress this comment.

[kraih]

I'd rather make it easier to write backend specific code. Like with a $minion->backend_name or so.

[guillomovitch]

The point here is precisely to avoid having to write backend-specific code, as the problem does not come from the backend.

A backend_name() method would have few added value compared to just checking backend class, as in this previous comment. A handle() method in each backend, tough, would be helpful. Even if Mojo::Pg, Mojo::mysql, etc don't implement a common interface, they have similar behaviour, allowing something such as:

my $handle = $app->minion->backend->handle; if ($handle->can('ping')) { Mojo::IOLoop->recurring($delay => sub ($loop) { $handle->ping; }); }

[kraih]

A handle() method in each backend, tough, would be helpful. Even if Mojo::Pg, Mojo::mysql, etc don't implement a common interface, they have similar behaviour, allowing something such as:

my $handle = $app->minion->backend->handle; if ($handle->can('ping')) { Mojo::IOLoop->recurring($delay => sub ($loop) { $handle->ping; }); }

That doesn't work because not all backends are DBI based. I guess then we can't do anything here.