SentinelManagedConnection searches for new master upon connection failure (#3560) #3601
Conversation
|
Hi @ManelCoutinhoSensei, thank you for your contribution! We’ll review your change soon(after PR #3596 :) ). |
|
It's been 2 months... do you have any updates @petyaslavova ? |
There was a problem hiding this comment.
Hi @ManelCoutinhoSensei,
Apologies for the late response. I’ve reviewed the code and added a few comments.
| @@ -294,13 +328,16 @@ def discover_master(self, service_name): | |||
| """ | |||
| collected_errors = list() | |||
| for sentinel_no, sentinel in enumerate(self.sentinels): | |||
| # print(f"Sentinel: {sentinel_no}") | |||
There was a problem hiding this comment.
Commented print statements should be removed.
| if str_if_bytes(await self.read_response()) != "PONG": | ||
| raise ConnectionError("PING failed") | ||
|
|
||
| if self.is_connected: |
There was a problem hiding this comment.
That’s quite a bit of code duplication. Perhaps the approach with a flag would be preferable after all. Since connect() is part of the interface, I think it’s better not to modify it directly. Instead, you can use the connect_with_health_check() method from AbstractConnection. You could introduce an additional flag to enable or disable retry_socket_connect, with a default value that preserves the current behaviour.
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 #3560. It should be reviewed after merging #3596