fix: do not handle errors during connection #616
Conversation
hauleth
force-pushed
the
do-not-handle-errors-in-postgres-libcluster-strategy
branch
from
198a011 to
b599f4a
Compare
| {:noreply, put_in(state.meta, meta)} | ||
| else | ||
| reason -> | ||
| Logger.error(state.topology, "Failed to connect to Postgres: #{inspect(reason)}") |
There was a problem hiding this comment.
What happens when this fails loudly does it take down the whole app with it?
There was a problem hiding this comment.
I would need to check exactly, but in theory, if the failure happen repeatedly, then it can cause whole application to go down.
There was a problem hiding this comment.
does it take down the whole app with it?
No, Supervisor will try to restart the Postgres strategy process, and after exhausting the retries, it will just leave it unstarted. But that change doesn’t make much sense anyway, because the node will be out of the cluster in case of a Postgres failure. You probably want to forward already assigned values, send a connect message after a timeout, and handle them more gracefully
hauleth
force-pushed
the
do-not-handle-errors-in-postgres-libcluster-strategy
branch
from
b599f4a to
c33107a
Compare
hauleth
force-pushed
the
do-not-handle-errors-in-postgres-libcluster-strategy
branch
2 times, most recently
from
9c386f8 to
4d8320e
Compare
In case of failure in the following cases, the node will not handle notifications because there is no retry logic. So yeah, there’s a potential place for a bug, but it doesn’t lead to leaking connections |
hauleth
force-pushed
the
do-not-handle-errors-in-postgres-libcluster-strategy
branch
4 times, most recently
from
a03fbbb to
1642320
Compare
This is potential source of a problem, as if there are is error in 2nd or 3rd clause of `with` then we are leaking connections which in the end may result in the connections exhaustion. Instead we should fail loudly that there is something wrong with the connections. It should also help with debugging potential problems.
hauleth
force-pushed
the
do-not-handle-errors-in-postgres-libcluster-strategy
branch
from
1642320 to
c7b4c06
Compare
|
@hauleth I've closed that PR because there's already one in progress that adds auto_reconnect. So, to fix the current issue, just need to add auto_reconnect to the notification connection |
3 participants
This is potential source of a problem, as if there are is error in 2nd or 3rd clause of
withthen we are leaking connections which in the end may result in the connections exhaustion. Instead we should fail loudly that there is something wrong with the connections. It should also help with debugging potential problems.