Support Polling Clients behind a load balancer with a Redis backed Pending Request Queue

[LukeButters]

Background

(In beta, don't use it in production!)

One of the issues with Polling Services in Halibut, is when we have a cluster of Clients (so many client nodes) and the polling service is configured to connect to one of those clients e.g. via a LB. The issue is the the client which has the work may not be the one that is connected to the service, and so the request is unable to be sent to the service.

The Redis Pending Request Queue solves this problem by having all Clients in the cluster, talk to a single Redis server. This allows any client to place a Request on to the Redis backed queue, and any client the service is connected to can retrieve that request from the queue.

This allows for multi node setups, where a polling service is connected to a single client yet any client can send work to the service.

See Redis Queue doco for more details.

How to review this PR

Quality ✔️

Pre-requisites

• I have read How we use GitHub Issues for help deciding when and where it's appropriate to make an issue.
• I have considered informing or consulting the right people, according to the ownership map.
• I have considered appropriate testing for my change.

[gb-8]

Can we use model retryability without depending on specific exception messages?

[LukeButters]

Added the following comment:

// Sometimes the error occurs NOT on the node executing the RPC, e.g. the Node talking to tentacle.
                // In that case we need to look at error messages, since we won't have the original exception type.
                // We will also need to check error messages any time Error Responses are raised rather than a raw exception
                // bubbling out of the QueueAndWait method.

If we could have checked the Exception type itself, feel free to make that change.

[gb-8]

Not awaited?

[LukeButters]

Yeah that does seem strange but as it turns out that is fine, we don't care how that goes just long as it gets disposed. I will make that clearer in the code:

// We created a queue that won't be used, dispose of it in the background.
                Task.Run(() => Try.IgnoringError(() => createdQueue.DisposeAsync()));

[gb-8]

We can just provide a constructor here to avoid the pragmas.

[LukeButters]

This file has a comment at the top describing why it is like it is.

I have also added:

// This class is copied from `MessageSerializer`, since this class tries to
        // use what it can from that battle tested class. That class had this envelope,
        // and so shall we, since we don't want to learn the hard way why it was like that :D.

I don't want to stray away from the MessageSerializer class, unless it gives us significant value.

[gb-8]

Since we're not relying in the async dispose pattern here, (i.e. we're calling DisopseAsync() directly), it would be more readable if we named this method with a more descriptive name for what it is doing.

Can it throw, given each task is already swallowing exceptions?
If not, we stop using Try.IgnoringError in client code that calls it.

[LukeButters]

I deleted this class.

Replaced the one usage with await Task.WhenAll(tasks.Select(t => Try.IgnoringError(() => t)));

Thanks revisiting this is useful.

[rhysparry]

✅ I've looked over this and have talked it through with Luke. Test coverage looks good and appropriate safety guards are in place.

Tests as they are should make refactoring safer in the future.