Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

1316: support resilient redis mode for cache util #1324

Closed
wants to merge 3 commits into from

Conversation

abarrak
Copy link

@abarrak abarrak commented Feb 10, 2022

This resolves #1316 and make it useful to use the cache without fail in case of network disconnect, etc.

Tests passing:
(results included in commit message)
Screen Shot 2022-02-10 at 12 07 17 PM

@abarrak abarrak requested a review from a team as a code owner February 10, 2022 09:12
unleashed
unleashed previously approved these changes Feb 10, 2022
@unleashed unleashed requested a review from eloycoto February 10, 2022 12:03
Copy link
Contributor

@eloycoto eloycoto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@kevprice83 I'm not sure if this affects you.

@abarrak
Copy link
Author

abarrak commented Feb 10, 2022

Thanks @eloycoto and @unleashed ..
Just covered both scenarios and made sure to keep backward compatibility as the default.

Tests run is passing:

$ make busted-util
EXTRA_CFLAGS="-DHAVE_EVP_KDF_CTX=1" /usr/local/openresty/luajit/bin/rover install --roverfile=gateway/Roverfile > /dev/null
/usr/local/openresty/luajit/bin/rover exec bin/busted "spec/threescale_utils_spec.lua" 
●●●
3 successes / 0 failures / 0 errors / 0 pending : 0.022514 seconds

@abarrak
Copy link
Author

abarrak commented Feb 11, 2022

Guys I thought again about current implementation and think it's better to keep _M.error() signature intact and introduce a new function (e.g. _M.silent_error()) using same signature ... and without nginx exit call.

This way will make the redis connect use either based on the resilient flag and without impacting original function usage.
Shared chunk in the function will be reused without duplication.

What do you think @unleashed @eloycoto @kevprice83 ?

Update:
New implementation is done and re-based all commits.

bash-4.4$ make busted-util
EXTRA_CFLAGS="-DHAVE_EVP_KDF_CTX=1" /usr/local/openresty/luajit/bin/rover install --roverfile=gateway/Roverfile > /dev/null
/usr/local/openresty/luajit/bin/rover exec bin/busted "spec/threescale_utils_spec.lua" 
●●●
3 successes / 0 failures / 0 errors / 0 pending : 0.02537 seconds

Tests passing:
```
$ make busted-util
/usr/local/openresty/luajit/bin/rover exec bin/busted "spec/threescale_utils_spec.lua"
●●●
3 successes / 0 failures / 0 errors / 0 pending : 0.02537 seconds
```
@kevprice83
Copy link
Member

In general I think this is fine and adds a useful option. We could probably look at extending error_handler in the future to map the same auth caching modes we support in the auth caching policy: allow, none, strict & resilient. That might also be of interest to you @abarrak in case you wanted more granular control over the auth caching decisions when using redis as a shared cache.

Unit Tests:
```
$ make busted-util
EXTRA_CFLAGS="-DHAVE_EVP_KDF_CTX=1" /usr/local/openresty/luajit/bin/rover install --roverfile=gateway/Roverfile > /dev/null
/usr/local/openresty/luajit/bin/rover exec bin/busted "spec/threescale_utils_spec.lua"
●●●
3 successes / 0 failures / 0 errors / 0 pending : 0.039009 seconds
```
@eguzki
Copy link
Member

eguzki commented May 12, 2022

@kevprice83 please review this PR and we decide to merge or close it

@abarrak
Copy link
Author

abarrak commented Apr 28, 2023

Hi @eguzki @kevprice83, could you decide on this PR clousre?
It was originally approved but got new changes after review.

@abarrak abarrak closed this May 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Threescale Util fails the current request if redis connection is not initiated successfully
5 participants