-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Fixing sentinel command execution to allow returning of actual responses when meaningful - behaviour controlled by 'return_responses' argument. #3191
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
Conversation
@dvora-h Can you please take a look. |
@vladvildanov Can you please take a look ? |
@mahigupta Thanks for your input! I'll also include @gerzse into this |
Hello, |
@mahigupta Looks fine 👌 Working on fixing CI, docker-compose was removed from default commands in Ubuntu 22.04 LTS runner |
@vladvildanov @gerzse Ci fixed now to go ahead with this PR ? |
This change will still need a few new unit tests to be added. |
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.
Pull Request Overview
This PR fixes the behavior of the sentinel command response in Redis by updating the execute_command function to correctly parse responses and returning either a boolean or the original response based on the provided options.
- Updates in both synchronous and asynchronous sentinel modules to pop "once" and "return_responses" from kwargs and return the processed response.
- Updates to the Redis commands for sentinel to pass the new execution options.
- An update to the CHANGES file documenting the fix.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
File | Description |
---|---|
redis/sentinel.py | Updated execute_command to handle return_responses and once logic. |
redis/commands/sentinel.py | Modified sentinel command method calls to include new options. |
redis/asyncio/sentinel.py | Asynchronous execute_command updated similar to its sync counterpart. |
CHANGES | Documented the fix for the sentinel command response behavior. |
…e type for sentinel's execute_command
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.
Pull Request Overview
This PR fixes how sentinel commands parse and return responses by introducing a return_responses
flag and removing the default True
return behavior. It also updates several sentinel command wrappers to leverage this new flag and expands test coverage with real-sentinel integration tests.
- Enhance
execute_command
(sync/async) to poponce
/return_responses
flags and return a list or boolean accordingly - Update sentinel command methods to use
once=True
and/orreturn_responses=True
where appropriate - Add
deployed_sentinel
fixtures and real-instance tests in both sync and asyncio suites
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
tests/test_sentinel.py | Added deployed_sentinel fixture and real-sentinel integration tests |
tests/test_asyncio/test_sentinel.py | Mirror sync changes for asyncio tests and real-instance coverage |
redis/sentinel.py | Refactored execute_command to support return_responses flag |
redis/asyncio/sentinel.py | Refactored async execute_command similarly |
redis/commands/sentinel.py | Updated sentinel_* wrappers to pass once /return_responses options |
Comments suppressed due to low confidence (2)
redis/commands/sentinel.py:15
- The method now returns a list of tuples (due to return_responses=True) rather than a single tuple; please update this docstring to reflect the new return type.
"""Returns a (host, port) pair for the given ``service_name``"""
redis/commands/sentinel.py:23
- This change makes sentinel_master return a list of dicts instead of a single dict, which is a breaking API change; consider documenting this behavior clearly or offering a non-breaking alternative.
def sentinel_master(self, service_name):
…uilt-in 'all' function for better performance and improved readability.
…nces is now optional. Fixed the pydocs to reflect the actual method behavior.
Pull Request check-list
Description of change
Fixes #3139
execute_command
function to parse sentinel command response correctly and return the actual responses when new argumentreturn_responses
is set to True.sentinel_get_master_addr_by_name
,sentinel_master
,sentinel_sentinels