-
Notifications
You must be signed in to change notification settings - Fork 3
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 retry object to the connection #65
Conversation
WalkthroughThe pull request introduces a modification to the connection handling logic in the Changes
Assessment against linked issues
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #65 +/- ##
=======================================
Coverage 92.27% 92.27%
=======================================
Files 35 35
Lines 2601 2601
=======================================
Hits 2400 2400
Misses 201 201 ☔ View full report in Codecov by Sentry. |
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.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- falkordb/falkordb.py (1 hunks)
Additional context used
GitHub Check: codecov/patch
falkordb/falkordb.py
[warning] 126-126: falkordb/falkordb.py#L126
Added line #L126 was not covered by tests
@@ -123,7 +123,7 @@ | |||
|
|||
if Is_Sentinel(conn): | |||
self.sentinel, self.service_name = Sentinel_Conn(conn, ssl) | |||
conn = self.sentinel.master_for(self.service_name, ssl=ssl) | |||
conn = self.sentinel.master_for(self.service_name, ssl=ssl, retry=retry) |
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.
Improve test coverage for the added retry functionality.
The addition of the retry
parameter to the master_for
method call is a positive change that can enhance the reliability of the Redis connection when using a sentinel. It aligns well with the PR objectives and has the potential to handle transient failures more effectively.
However, the static analysis tool has flagged that the added line is not covered by tests. To ensure the retry mechanism behaves as expected and to maintain a high level of code quality, it is crucial to add test cases that validate the functionality of the retry parameter.
Do you want me to generate the missing test cases or open a GitHub issue to track this task?
Tools
GitHub Check: codecov/patch
[warning] 126-126: falkordb/falkordb.py#L126
Added line #L126 was not covered by tests
fix #64
Added a retry object for the Redis connection that is created after getting the master connection.
Summary by CodeRabbit
New Features
Bug Fixes