-
Notifications
You must be signed in to change notification settings - Fork 479
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
add some redis client config that may be commonly used #763
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I love seeing more connection settings coming in this PR! I'd like just a few changes for consistency with the duration-based settings.
…nterval and processingTimeout are and remove the InSec suffix
…s backOffMinInterval backOffMaxInterval. to be consistent with other components
@Taction Thanks for all your work on this. I know there have been a few iterations now. Two things:
Context: In Where I steered you wrong: All the settings I had you prefix with |
@pkedy :
As for the prefix, is
What's more, in my opinion both |
For 1, the For 2, I think we are in agreement! Also, please mention the docs PR in this PR or visa versa. Thanks! |
LGTM |
@Taction To @mukundansundar 's point in your docs PR, we need an issue created as this is a pretty good sized enhancement. We do this for tracking things int the RELEASE NOTES. |
Codecov Report
@@ Coverage Diff @@
## master #763 +/- ##
==========================================
+ Coverage 32.78% 32.86% +0.08%
==========================================
Files 97 97
Lines 8744 8847 +103
==========================================
+ Hits 2867 2908 +41
- Misses 5616 5649 +33
- Partials 261 290 +29
Continue to review full report at Codecov.
|
@Taction I added RELEASE NOTES to the description of this PR which our release process also picks up. So in the interest of moving this along, no need to create an issue. Just know for the future that large changes need an issue. |
Description
add some config that can be set from metadata for redis client.
Issue reference
Checklist
Please make sure you've completed the relevant tasks for this PR, out of the following list:
RELEASE NOTE: Added commonly used Redis client settings