Skip to content

[Messenger] Add support for multiple Redis Sentinel hosts #51687

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

Merged
merged 1 commit into from
Sep 20, 2023

Conversation

digilist
Copy link
Contributor

Q A
Branch? 6.4
Bug fix? no
New feature? yes
Deprecations? no
Tickets n/a
License MIT
Doc PR n/a

Similar to #47003 which added support for multiple Redis Sentinel hosts for the Cache component, this PR adds support for multiple Sentinel hosts for the Messenger component.

This PR is inspired by the implementation in the cache component and works very similar. A DSN could look like this: redis:?host[localhost:26377]&host[localhost:26379]&sentinel_master=db.

I changed the Sentinel host environment variable for the ingegration to an invalid host at. As a result I noticed that Relay also fails in such case and so I expanded my earlier changes from #51598 to also ignore unreachable hosts with the Relay extension.

@digilist digilist force-pushed the messenger-multiple-sentinel branch 2 times, most recently from 3e13640 to 578a794 Compare September 18, 2023 21:19
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM thanks, I just have one minor comment

@digilist digilist force-pushed the messenger-multiple-sentinel branch from 578a794 to 3380518 Compare September 19, 2023 09:56
@nicolas-grekas
Copy link
Member

Thank you @digilist.

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Sep 28, 2023

@digilist there are some failures on our integration tests that might be related to this PR. Would you have some availability to have a look please? 🙏

@digilist
Copy link
Contributor Author

Yes, sure. It looks like a merge conflict between #51683 and this PR was resolved incorrectly, as there are now two connection attempts to Sentinel:

if (version_compare(phpversion('redis'), '6.0.0', '>=') && $isRedisExt) {
$options = [
'host' => $host,
'port' => $port,
'connectTimeout' => $params['timeout'],
'persistent' => $params['persistent_id'],
'retryInterval' => $params['retry_interval'],
'readTimeout' => $params['read_timeout'],
];
if ($passAuth) {
$options['auth'] = $params['auth'];
}
$sentinel = new \RedisSentinel($options);
} else {
$extra = $passAuth ? [$params['auth']] : [];
$sentinel = new $sentinelClass($host, $port, $params['timeout'], (string) $params['persistent_id'], $params['retry_interval'], $params['read_timeout'], ...$extra);
}
try {
$sentinel = new $sentinelClass($host, $port, $params['timeout'], (string) $params['persistent_id'], $params['retry_interval'], $params['read_timeout'], ...$extra);
if ($address = $sentinel->getMasterAddrByName($params['redis_sentinel'])) {
[$host, $port] = $address;
}
} catch (\RedisException|\Relay\Exception $redisException) {
}

I'll provide a PR.

nicolas-grekas added a commit that referenced this pull request Sep 28, 2023
This PR was merged into the 6.4 branch.

Discussion
----------

[Cache] Fix two initializations of Redis Sentinel

| Q             | A
| ------------- | ---
| Branch?       | 6.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | n/a
| License       | MIT

After #51687 and #51683 have been merged, there are now two initializations of Redis Sentinel, which also leads to the integration test failure mentioned in [this comment](#51687 (comment)). This PR fixes this.

Commits
-------

2d23a1b [Cache] Fix two initializations of Redis Sentinel
This was referenced Oct 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants