[WPB-18631] Robustness against rabbitmq outage #4632
Conversation
zebot
added
the
ok-to-test
fisx
force-pushed
the
WPB-18443-robustness-against-rabbitmq-outage
branch
4 times, most recently
from
a47dc6c to
449ea6e
Compare
fisx
left a comment
fisx
left a comment
There was a problem hiding this comment.
looks ready to merge to me!
fisx
force-pushed
the
WPB-18443-robustness-against-rabbitmq-outage
branch
from
19f1884 to
6e6f888
Compare
Does this really make a difference? Can the warp thread crash while the waitCatch is still alive? If not we could revert the last chunk of change, but I'd like to keep the cleanupCallback thingy.
Make connections easier to indentify in the web-ui.
The supervisor creates an Async and the worker returns one... This cannot work. Remove one Async instantiation.
fisx
force-pushed
the
WPB-18443-robustness-against-rabbitmq-outage
branch
from
0e7a062 to
573ace3
Compare
integration/test/Test/Events.hs
Outdated
| -- Some times RabbitMQ still remembers connections from previous uses of the | ||
| -- dynamic backend. So we wait to ensure that we kill connection only for our | ||
| -- current. | ||
|
|
There was a problem hiding this comment.
Do we need this empty line?
| statuses = undefined | ||
| rabbitmqAdminClient = Just $ mockRabbitMqAdminClient mockAdmin | ||
| rabbitmqVHost = "test-vhost" | ||
| amqpEP = Q.AmqpEndpoint undefined undefined "test-vhost" undefined |
There was a problem hiding this comment.
Can't the whole amqpEP be undefined (instead of having almost al fields of AmqpEndpoint being undefined)? 🤔
There was a problem hiding this comment.
i don't know, we did set rabbitmqVHost before, and i didn't see any reason to change that and deal with whatever happens?
There was a problem hiding this comment.
Finally, it's not really important. 😄
One could also ask if AppT isn't too big if we only need the vhost. Or, if getRemoteDomains :: RabbitMqAdmin.AdminAPI (Servant.AsClientT IO) -> Text {- vhosy -} -> AppT IO [Domain] wouldn't be sufficient. Hower, not our construction place right now 😉
| -- Notification pusher thread | ||
| Log.info $ Log.msg (Log.val "Cancelling the notification pusher thread") | ||
| readIORef notifChanRef >>= traverse_ \chan -> do | ||
| Log.info $ Log.msg (Log.val "Got channel") |
There was a problem hiding this comment.
The channel's name might be interesting here as well.
There was a problem hiding this comment.
i can only find channelID, and that is only exported from Network.API.Internal, which is not exposed in the cabal file. any hints where to find the name?
There was a problem hiding this comment.
Ah, that mess. Yeah, I remember we've tried that before ... 🙄
Well, then, we may leave this for now.
| async $ | ||
| liftIO $ | ||
| openConnectionWithRetries env.logger rabbitmqOpts $ | ||
| openConnectionWithRetries env.logger "BackendNotificationPusher" rabbitmqOpts $ -- can we find a bug here, too? |
There was a problem hiding this comment.
What about this? Shall we search a potential bug?
There was a problem hiding this comment.
yes, but not in this PR.
There was a problem hiding this comment.
Hm, maybe transform this into a FUTUREWORK: ... or a ticket? 🤔
Co-authored-by: Sven Tennie <[email protected]>
supersven
left a comment
supersven
left a comment
There was a problem hiding this comment.
Unfortunately, it's pretty hard be ensure that all this is free of deadlocks, because there are locks in many places.
I haven't found any so far. So, I'm approving. ✔️
…s-against-rabbitmq-outage' into WPB-18443-robustness-against-rabbitmq-outage
fisx
changed the title
akshaymankar
left a comment
akshaymankar
left a comment
There was a problem hiding this comment.
Let's see if #4652 works.
|
Superseded by #4652 |
https://wearezeta.atlassian.net/browse/WPB-18631
(found working on https://wearezeta.atlassian.net/browse/WPB-18443)
Checklist
changelog.d