Fixing sentinel command execution to allow returning of actual responses when meaningful - behaviour controlled by 'return_responses' argument. by mahigupta · Pull Request #3191 · redis/redis-py (original) (raw)
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
Pull Request check-list
- Do tests and lints pass with this change?
- Do the CI tests pass with this change (enable it first in your forked repo and wait for the github action build to finish)?
- Is the new or changed code fully tested?
- Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?
- Is there an example added to the examples folder (if applicable)?
Description of change
Fixes #3139
- Made change to
execute_commandfunction to parse sentinel command response correctly and return the actual responses when new argumentreturn_responsesis set to True. - This behaviour is exposed for several sentinel commands -
sentinel_get_master_addr_by_name,sentinel_master,sentinel_sentinels - The default behaviour is preserved, so this won't be a breaking change.
@mahigupta Looks fine 👌 Working on fixing CI, docker-compose was removed from default commands in Ubuntu 22.04 LTS runner
This change will still need a few new unit tests to be added.
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
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_responsesflags and return a list or boolean accordingly - Update sentinel command methods to use
once=Trueand/orreturn_responses=Truewhere appropriate - Add
deployed_sentinelfixtures 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.
Fixing sentinel command response Fixing sentinel command execution to allow returning of actual responses when meaningful - behaviour controlled by 'return_responses' argument.
ManelCoutinhoSensei pushed a commit to ManelCoutinhoSensei/redis-py that referenced this pull request
…ses when meaningful - behaviour controlled by 'return_responses' argument. (redis#3191)
Fixing sentinel command response
Linter check pass
Applying review comments and fixing some issues.
Fix several spelling mistakes
Adding new sentinel integration tests. Fixing the boolean return value type for sentinel's execute_command
Replacing usage of reduce in the boolean return types with python's built-in 'all' function for better performance and improved readability.
Changing the default behavior to be as before, returning of the responces is now optional. Fixed the pydocs to reflect the actual method behavior.
Co-authored-by: petyaslavova petya.slavova@redis.com
ManelCoutinhoSensei pushed a commit to ManelCoutinhoSensei/redis-py that referenced this pull request
…ses when meaningful - behaviour controlled by 'return_responses' argument. (redis#3191)
Fixing sentinel command response
Linter check pass
Applying review comments and fixing some issues.
Fix several spelling mistakes
Adding new sentinel integration tests. Fixing the boolean return value type for sentinel's execute_command
Replacing usage of reduce in the boolean return types with python's built-in 'all' function for better performance and improved readability.
Changing the default behavior to be as before, returning of the responces is now optional. Fixed the pydocs to reflect the actual method behavior.
Co-authored-by: petyaslavova petya.slavova@redis.com
petyaslavova added a commit that referenced this pull request
…ses when meaningful - behaviour controlled by 'return_responses' argument. (#3191)
Fixing sentinel command response
Linter check pass
Applying review comments and fixing some issues.
Fix several spelling mistakes
Adding new sentinel integration tests. Fixing the boolean return value type for sentinel's execute_command
Replacing usage of reduce in the boolean return types with python's built-in 'all' function for better performance and improved readability.
Changing the default behavior to be as before, returning of the responces is now optional. Fixed the pydocs to reflect the actual method behavior.
Co-authored-by: petyaslavova petya.slavova@redis.com