Skip to content
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

[8.x] Add phpredis serialization and compression support to the cache component #40569

Closed

Conversation

TheLevti
Copy link
Contributor

@TheLevti TheLevti commented Jan 23, 2022

This is a follow up of #36412 and #40282 which added phpredis serialization and compression support to the redis component.

The Problem

Laravel's Cache component is built on top of the Redis component. The Cache component serializes on its own before sending data to the Redis component, which is a problem when we use phpredis with serialization enabled.

The Solution

When phpredis is used together with serialization enabled, skip the serialization part on the Cache component.

Details

This pull request adds these capabilities now to the cache component as there is some custom logic sitting on top of the redis component. The changes include prevention of double serialization/deserialization, proper eval argument packing and a reworked phpredis test suite, which lets all test that use redis run with all kind of different connection setups using data providers.

Note: I explicitly skipped also upgrading the queue component in this pull request. A queue component upgrade pull request will follow after this one. Until then redis queues do not support phpredis compression and serialization functionality yet. I was almost done with also the queue adjustments, but noticed that we do some LUA json decoding/encoding to increment the attempt counter, which I would need to move to a different level as phpredis will serialize/compress with different set of algorithms. Can you please suggest someone who has in depth knowledge of the php redis queue implementation in laravel as I have some concrete questions regarding doing changes there.

@TheLevti TheLevti force-pushed the feature/improve-phpredis-usage branch 6 times, most recently from 511af94 to 864b41c Compare January 23, 2022 13:51
@TheLevti TheLevti marked this pull request as draft January 23, 2022 13:56
@TheLevti TheLevti force-pushed the feature/improve-phpredis-usage branch 5 times, most recently from b93823f to 40eddad Compare January 23, 2022 18:28
@TheLevti TheLevti marked this pull request as ready for review January 23, 2022 18:28
@TheLevti TheLevti force-pushed the feature/improve-phpredis-usage branch 9 times, most recently from dedf959 to 8fa3540 Compare January 23, 2022 20:31
@driesvints
Copy link
Member

Is this still needed after @tillkruss' PR? #40571

@TheLevti
Copy link
Contributor Author

The pull request as a whole, yes. The flush fix not, I will rebase my pull request on top of latest 8.x later today.

@TheLevti TheLevti force-pushed the feature/improve-phpredis-usage branch 3 times, most recently from 1969e67 to 808ce4c Compare January 24, 2022 10:32
- Improved the RedisStore to support phpredis serialization and compression settings.
- Improve test suite related to phpredis.
@TheLevti TheLevti force-pushed the feature/improve-phpredis-usage branch from 808ce4c to b27163e Compare January 24, 2022 10:38
@TheLevti
Copy link
Contributor Author

Is this still needed after @tillkruss' PR? #40571

Ok rebased, ready for review.

],
];

switch ($connection) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explain what's going on in this switch?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This trait is a helper to give any test the ability to quickly generate and clean up phpredis and predis connections. Either from this predefined list (most critical settings) or by providing a custom configuration array. This is also used to provide dataProviders for tests to not loop through 25 different connections as a single test, but to have 1 test per connection, which is cleaner and allows to pin point which connection caused a failing test.

@taylorotwell
Copy link
Member

Honestly I don't have the time or knowledge to maintain anymore complicated code around Redis compression / serialization. If you need such functionality I suggest writing your own cache driver, etc.

@TheLevti
Copy link
Contributor Author

TheLevti commented Jan 24, 2022

Honestly I don't have the time or knowledge to maintain anymore complicated code around Redis compression / serialization. If you need such functionality I suggest writing your own cache driver, etc.

That is very unfortunate and I would be happy to properly support/maintain this component. Without further additions, the previous compression/serialization support was kind of pointless to add. In this pull request I reworked the redis test suite to thoroughly cover all uses of redis and redis connection combinations in a clean way (using data providers and named data points) as the existing tests are incomplete and quite messy. Any component that is using redis, is running through this list of connections and I made sure that whatever changes on phpredis or a related laravel class, will certainly uncover issues by failing one of those tests, which explains the number of changed files (also fixed some other non redis related flaky tests).

The Cache component support is just one or two files that needed some tweaks in some of their methods (RedisStore.php for example). I would understand to not add any Queue additions as its more complex (here I would go with a dedicated driver), for the Cache component those are rather trivial changes that bring big benefits as Redis is mostly used for caching.

If you really see no way to include this changes to add extended phpredis support, I will proceed with a separate driver as you suggested, because this features bring some strong performance additions for advanced applications.

@TheLevti
Copy link
Contributor Author

TheLevti commented Jan 24, 2022

I could also offer to split this pull request into two, first the test suite rework as a preparation step for any future redis changes and second on top of it the rather small adjustments of the Cache component. This way it would be easier for you guys to understand the required/proposed changes.

@TheLevti
Copy link
Contributor Author

I could also offer to split this pull request into two, first the test suite rework as a preparation step for any future redis changes and second on top of it the rather small adjustments of the Cache component. This way it would be easier for you guys to understand the required/proposed changes.

@taylorotwell @tillkruss can you tell me whether this would be fine?

@tillkruss
Copy link
Contributor

@TheLevti I'm a heavy user of compressions + igbinary myself. I'd be happy to take a look at smaller PRs and make sure they are easy to understand before opening them here.

@TheLevti
Copy link
Contributor Author

TheLevti commented Feb 4, 2022

@TheLevti I'm a heavy user of compressions + igbinary myself. I'd be happy to take a look at smaller PRs and make sure they are easy to understand before opening them here.

Added a new pull request that just improves the redis tests first #41718

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants