Add Redis readiness verification (#3555) #3596
Conversation
|
Hi @ManelCoutinhoSensei, thank you for your contribution! We’ll review your change soon. |
|
@petyaslavova you need to rerun the failed tests due to the test suit flakiness |
|
Hi @petyaslavova , any news about this one? |
|
Hi @ManelCoutinhoSensei, I have started the review, but couldn't complete it. |
|
Should I solve the conflicts because the PR reviews are going to be done this week or should I postpone it? |
Hi @ManelCoutinhoSensei, please wait for a while with the conflict solving. We first need to address one more planned change related to the async cluster, and then I will be able to progress on this PR. |
|
Hey @petyaslavova! 🙌 Just checking in to see if there are any updates or an estimated timeline for moving forward with this PR. Totally understand things can get busy — just wanted to make sure it’s still on your radar. |
|
|
There was a problem hiding this comment.
Hi @ManelCoutinhoSensei,
Apologies for the delay in reviewing the PR. I've added some comments and suggestions for changes that I believe will help make the code more maintainable in the long term. While the comments are placed in the sync client/connection code, they apply to the async parts as well.
| @@ -933,7 +968,6 @@ async def _connect(self): | |||
| reader, writer = await asyncio.open_unix_connection(path=self.path) | |||
| self._reader = reader | |||
| self._writer = writer | |||
| await self.on_connect() | |||
There was a problem hiding this comment.
Why do you remove this call?
| # Doing handshake since connect and send operations work even when Redis is not ready | ||
| if self.check_ready: | ||
| try: | ||
| ping_parts = self._command_packer.pack("PING") |
There was a problem hiding this comment.
I think it would be better to replace this block with a call to self._send_ping(), and refactor that method to accept an additional argument—something like accept_no_auth_response=False by default. This way, you can reuse the existing logic without duplicating the socket write/read operations, making the code cleaner and easier to maintain.
| @@ -203,6 +203,7 @@ def __init__( | |||
| encoding: str = "utf-8", | |||
| encoding_errors: str = "strict", | |||
| decode_responses: bool = False, | |||
| check_ready: bool = False, | |||
There was a problem hiding this comment.
Please add details about this setting to the pydocs. Also, I would suggest changing the name so that it would be easier to understand its purpose just by looking at the name, something like check_server_ready, or on_connect_check_server_ready
| @@ -378,12 +380,35 @@ def connect(self): | |||
| "Connects to the Redis server if not already connected" | |||
| self.connect_check_health(check_health=True) | |||
|
|
|||
| def _connect_check_ready(self): | |||
There was a problem hiding this comment.
Please move this method after connect_check_health
Pull Request check-list
Please make sure to review and check all of these items:
NOTE: these things are not required to open a PR and can be done
afterwards / while the PR is open.
Description of change
Fixes #3555 (specially the comment on that issue)
Please check if you'd rather run the tests with
check_ready=Trueby default (I also tested that) and if you think that any other tests deserve a second version with this flag set.