8000 [Cache] Add Redis Relay support by ostrolucky · Pull Request #48930 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Cache] Add Redis Relay support #48930

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
Jan 26, 2023
Merged

[Cache] Add Redis Relay support #48930

merged 6 commits into from
Jan 26, 2023

Conversation

ostrolucky
Copy link
Contributor
Q A
Branch? 6.3
Bug fix? no
New feature? yes
Deprecations? no
Tickets
License MIT
Doc PR

This PR adds support for Relay, a next-gen Redis client written in C by the makers of PhpRedis and Predis. It’s built as a drop-in replacement for PhpRedis with a backwards compatible interface for easy adoption. Relay is significantly faster than existing clients by leveraging Redis 6's client-side-caching.

While Relay is still on 0.x (pending the addition of cluster support in a few weeks for a 1.0 tag), it’s interface is stable and it’s heavily used in production deployments.

Similarly, I've also added Relay support to most popular symfony redis bundle, snc/redis-bundle. But to be able to support these new Redis instances in Symfony components as a Cache, Lock and so on, support had to be added here as well.

Since method and constant declarations are compatible with PhpRedis, I've opted into reusing most of the code instead of creating completely new adapters, similarly as was done in case of Predis+PhpRedis. At the same time, I made it a goal not having to have PhpRedis installed in case somebody wishes to use Relay, which explains things like conditional fetch of value constants.

@ostrolucky ostrolucky requested a review from jderusse as a code owner January 9, 2023 19:22
@carsonbot carsonbot added this to the 6.3 milestone Jan 9, 2023
@ostrolucky ostrolucky changed the title [Cache] Add Relay support Add Relay support Jan 9, 2023
@ostrolucky ostrolucky changed the title Add Relay support Add Redis Relay support Jan 9, 2023
@tillkruss
Copy link

Ping @michael-grunder.

nicolas-grekas
nicolas-grekas reviewed Jan 11, 2023
View reviewed changes
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.

Thanks for working on this!
What's the plan for cluser/array clients?
For the proxy, it'd be great to amend RedisProxiesTest.php to sync it if needed.

8000
@tillkruss
Copy link

What's the plan for cluser/array clients?

We're actively working on Cluster support, it will go into the 1.0 release in a few weeks. I'm not sure about RedisArray, I'll talk to @michael-grunder on whether we want to port that over, since there are about 2 production installs AFAIK.

@michael-grunder
Copy link

My preference would be to not replicate RedisArray, as we kind of want to deprecate the functionality even in PhpRedis. That said, we could implement the API if there is sufficient demand. It's much simpler than Cluster.

As @tillkruss said, we're working on Cluster support now. Feel free to mention any quality-of-life features you'd like to see.

@nicolas-grekas
Copy link
Member

Thanks for the hints. Another thing I'm wondering about is ext-redis v6: I'm seeing active dev on it. What's the plan related to that + relay?

@michael-grunder
Copy link

For now, I'm just making sure that the two extensions remain compatible.

@ostrolucky ostrolucky force-pushed the relay branch 6 times, most recently from 35462a6 to 8d7c7ba Compare January 12, 2023 23:38
@ostrolucky
Copy link
Contributor Author

All feedback addressed

@nicolas-grekas
Copy link
Member

One thing I'm wondering is: did you consider keeping the same class names as phpredis?
Because if there are no differences API-wise, why not?
Couldn't relay be released as phpredis v5.4?

@ostrolucky ostrolucky force-pushed the relay branch 3 times, most recently from f0a0725 to 8ea5149 Compare January 13, 2023 10:17
@ostrolucky
Copy link
Contributor Author

I've rebased the branch to solve the conflicts. Can we expect the merge soon? I promise I'll create a PR with migration to shivammathur/setup-php when they release the version ;)

@nicolas-grekas
Copy link
Member
nicolas-grekas commented Jan 22, 2023 •
edited
Loading

@michael-grunder @tillkruss any hints about my previous comment? Why a new set of symbols? Why not release as phpredis v5.4?

nicolas-grekas
nicolas-grekas approved these changes Jan 22, 2023
View reviewed changes
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!

@carsonbot carsonbot changed the title Add Redis Relay support [Cache] Add Redis Relay support Jan 22, 2023
@tillkruss
Copy link

One thing I'm wondering is: did you consider keeping the same class names as phpredis?

We did, but that would mean PhpRedis no Relay can't be installed at the same time and easily switched between.

Because if there are no differences API-wise, why not?

We did initially in 2020 consider this, but leveraging server-assisted client-side caching required starting from scratch, hence a separate name, new codebase, etc.

PhpRedis has a lot of legacy junk that we didn't want to bring over.

Couldn't relay be released as phpredis v5.4?

People don't like change, PhpRedis will remain FOSS and we'll suggest users to move to Relay once 1.0 is out.

Predis and PhpRedis will remain maintained of course, but who doesn't want a more modern, faster client ✌️

@nicolas-grekas
Copy link
Member

(rebase needed 🙏 + don't miss #48930 (comment))

@ostrolucky ostrolucky force-pushed the relay branch 3 times, most recently from afb9fd9 to ad9c24d Compare January 23, 2023 21:39
@ostrolucky
Copy link
Contributor Author

done

@ostrolucky
Copy link
Contributor Author

Psalm found 1 relevant issue (missing \Relay\ in \Relay\Exception) so I fixed that. Not sure what is a desired fix for the other issues though. I think ideally, here

if ($this->redis instanceof \Predis\ClientInterface) {
   $prefix = $this->redis->getOptions()->prefix ? $this->redis->getOptions()->prefix->getPrefix() : '';
  $prefixLen = \strlen($prefix ?? '');
}

before calling getPrefix, we should check if prefix is instanceof KeyPrefixProcessor.

And for other issues, ProxyHelper should be fixed to account for missing magic methods in parent.

I think none of these are related to this PR.

@nicolas-grekas
Copy link
Member

Thank you @ostrolucky.

@nicolas-grekas nicolas-grekas mentioned this pull request Jan 26, 2023
Closed
@nicolas-grekas nicolas-grekas merged commit c223437 into symfony:6.3 Jan 26, 2023
@ostrolucky ostrolucky mentioned this pull request Jan 27, 2023
Merged
fabpot added a commit that referenced this pull request Jan 27, 2023
@fabpot
minor #49125 Utilize shivammathur/setup-php to install Relay extensio…
f06554b 
…n (ostrolucky)

This PR was merged into the 6.3 branch.

Discussion
----------

Utilize shivammathur/setup-php to install Relay extension

| Q             | A
| ------------- | ---
| Branch?       | 6.3
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Tickets       | Follow-up to #48930
| License       | MIT
| Doc PR        |

This is possible since https://github.com/shivammathur/setup-php/releases/tag/2.24.0. A bit unfortunate timing since they tagged it one day after we merged relay, but no biggie.

Commits
-------

b2b0a75 Utilize shivammathur/setup-php to install Relay extension
@nicolas-grekas
Copy link
Member

@tillkruss I'm sorry but I didn't manage to find the source code of the Relay extension. Can you please let me know the link to it?

@tillkruss
Copy link

@nicolas-grekas We haven't published the source of Relay, because of it's commercial tier. We may in the future. As for security, we've done code audits our larger customers in the past with, if that's something you'd like.

@magnusnordlander magnusnordlander mentioned this pull request Apr 20, 2023
Closed
@fabpot fabpot mentioned this pull request May 1, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tillkruss tillkruss tillkruss left review comments

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

@jderusse jderusse Awaiting requested review from jderusse jderusse is a code owner

Assignees
No one assigned
Labels
Cache Feature Status: Reviewed
Projects
None yet
Milestone
6.3
Development

Successfully merging this pull request may close these issues.
5 participants
@ostrolucky @tillkruss @michael-grunder @nicolas-grekas @carsonbot
0