3scale / apisonator

Red Hat 3scale API Management Apisonator backend
https://3scale.net
Apache License 2.0
36 stars 27 forks source link

THREESCALE-8404: Bump redis to 5.2.0 #384

Closed jlledom closed 1 month ago

jlledom commented 1 month ago

This comes from https://github.com/3scale/apisonator/pull/350. That PR is too big, it has a lot of outdated info and so many comments and commits it could lead to confusion. I decided to split it into smaller PRs easier to review and understand.

This PRs updates the redis gem to 5.2.0 but it doesn't include any new functionality compared to master.

Commits:

jlledom commented 1 month ago

This comes from https://github.com/3scale/apisonator/pull/350#pullrequestreview-2053678308

IIRC, when @fetch_timeout is reached, the method will return nil, not raise. A read timeout on the other hand is a low-level TCP timeout that shouldn't happen regularly.

On the other hand, we don't need to be very much bothered by a connection timeout, if we can just reconnect. It's good to be resilient in case of network glitches.

I would suggest though to log the error, can be without a trace, just dome text like:

Que redis (ip/hostname of server if possible) connection timeout at #{FILE}:#{LINE}

In this way we can have an indication of network stability and see whether particular servers or locations are more problematic. This will help debugging and improving the cluster setup.

@akostadinov You're right, the docs say it should return nil, however it raises an exception when connecting to sentinels. I opened an issue upstream to fix it: https://github.com/redis/redis-rb/issues/1279

In the meantime we must capture the exception. And I don't think we need to log it because it would flood the logs with this error happening all the time, since it's the expected behavior.

jlledom commented 1 month ago

From https://github.com/3scale/apisonator/pull/350#discussion_r1599011490:

Why can provider key be nil? Should we conditionally add or should we raise an error?

@akostadinov I honestly don't remember why I did this, and it indeed looks wrong. I'm removing this line for now, let's see if something breaks.

jlledom commented 1 month ago

From #350 (comment):

Why can provider key be nil? Should we conditionally add or should we raise an error?

@akostadinov I honestly don't remember why I did this, and it indeed looks wrong. I'm removing this line for now, let's see if something breaks.

@akostadinov I found why I did it. It's because some tests fail. redis-rb 5 doesn't accept nils as parameters for it's redis calls, only strings. However, I think it's the tests what were wrong, not the code. I pushed some changes (https://github.com/3scale/apisonator/pull/384/commits/b4c0c8788a21526d54377679c8933b9a39f35f31)

mayorova commented 1 month ago

Looks good @jlledom great job! :clap:

I'm just concerned about that change with the provider_key in tests. Otherwise, all looks nice.