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

[10.x] Remove serializer and compression for RedisQueue #48180

Closed
wants to merge 2 commits into from

Conversation

SudoGetBeer
Copy link

This PR should address #47356 and #47578.

Alternatively, maybe reference in the documentation that you should use a separate database.redis connection like that:

        'queue' => [
            'url' => env('REDIS_URL'),
            'host' => env('REDIS_HOST', '127.0.0.1'),
            'username' => env('REDIS_USERNAME'),
            'password' => env('REDIS_PASSWORD'),
            'port' => env('REDIS_PORT', '6379'),
            'database' => env('REDIS_DB', 0),
            'options'=> [
                'serializer' => 0,
                'compression' => 0,
            ],
        ],

and then you can use that in the queue.php config file.

(First PR here hope I didn't miss anything)

@taylorotwell
Copy link
Member

taylorotwell commented Aug 27, 2023

A bit concerned about caching off this connection on this class and what implications that has for long-lived applications like Octane, queues, etc. Is there a solution that doesn't require that kind of caching? Please mark as ready for review if you need my input again.

@taylorotwell taylorotwell marked this pull request as draft August 27, 2023 21:11
@SudoGetBeer
Copy link
Author

SudoGetBeer commented Aug 28, 2023

@taylorotwell
Ah, I understand. I see these options:

  1. Just add info to the docs.
  2. Show an error when you try to use the serializer or compression for the redis queue connection.
  3. I could change the PR so that RedisManager.php accepts a second parameter so we can save the redis queue connection with a different name. And I assume that then the purge function would be able to handle that for Octane etc. Don't know if this would be a breaking change.
  4. I don't know how big the impact would be if we create a new connection everytime, but I assume there is one.
  5. I found this comment by @tillkruss Error removing jobs from the reserved queue #47356 (comment) so maybe I could try to rewrite the lua code to use transactions.

I think that 2. would be easiest to realize. Don't know if there are other benefits for 5. Maybe @tillkruss could elaborate on that.

Will open this for review just for your feedback, Taylor.

@SudoGetBeer SudoGetBeer marked this pull request as ready for review August 29, 2023 09:20
if ($this->redisConnection instanceof PhpRedisConnection) {
$this->redisConnection = $this->redis->resolve($this->connection);

if (defined('\Redis::OPT_COMPRESSION') && $this->redisConnection->client()->getOption(\Redis::OPT_COMPRESSION) !== \Redis::COMPRESSION_NONE) {
Copy link
Contributor

Choose a reason for hiding this comment

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

OPT_COMPRESSION is always defined.

if (defined('\Redis::OPT_COMPRESSION') && $this->redisConnection->client()->getOption(\Redis::OPT_COMPRESSION) !== \Redis::COMPRESSION_NONE) {
$this->redisConnection->client()->setOption(\Redis::OPT_COMPRESSION, \Redis::COMPRESSION_NONE);
}
if (defined('\Redis::OPT_SERIALIZER') && $this->redisConnection->client()->getOption(\Redis::OPT_SERIALIZER) !== \Redis::SERIALIZER_NONE) {
Copy link
Contributor

Choose a reason for hiding this comment

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

OPT_SERIALIZER is always defined, so is SERIALIZER_NONE and COMPRESSION_NONE.

$this->redisConnection = $this->redis->resolve($this->connection);

if (defined('\Redis::OPT_COMPRESSION') && $this->redisConnection->client()->getOption(\Redis::OPT_COMPRESSION) !== \Redis::COMPRESSION_NONE) {
$this->redisConnection->client()->setOption(\Redis::OPT_COMPRESSION, \Redis::COMPRESSION_NONE);
Copy link
Contributor

Choose a reason for hiding this comment

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

I wouldn't access \Redis directly here, you can make $this->redisConnection->client() a variable and reference the constant on it.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for your input. Since I will have to change that code, I will wait for Taylors answer.
Maybe you can give me/us a hint why the MULTI (transaction) would be better than the currently used Lua script.
Thanks again!

Copy link
Contributor

Choose a reason for hiding this comment

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

Because multi() works with serializer and compression.

@taylorotwell
Copy link
Member

Honestly seems like we could just add note to the docs.

@cyppe
Copy link

cyppe commented Oct 2, 2023

I have same issue. Does this mean there is no way to use compression/serialize for one connection, and using queues on another connection?

My current workaround is to use database for jobs, but it's not very scalable.

So for me the nicest solution would be if it's possible to set diffent options per redis connection so the connection for queue jobs will not use compression.

@cyppe
Copy link

cyppe commented Oct 2, 2023

I see now that it actually worked to specify options inside the queue connection block. So I think that should also be added to docs that it's how you can solve it. So thanks SudoGetBeer for mentioning it as a solution,

@Mdhesari
Copy link

Mdhesari commented Oct 2, 2023

@cyppe could you explain more?

@cyppe
Copy link

cyppe commented Oct 2, 2023

I just mean I was not aware of the possibility to set 'options' specifically for one redis connection. I thought as all examples I can find is that options is in top of redis config block, and not inside each connection.

So I had the same issue where queues did not work. But now I added options for my queue connection to disable compression only for that connection and now it works as expected.

`
// database.php config

'redis' => [

    'client' => env('REDIS_CLIENT', 'phpredis'),

    'options' => [
        'cluster' => env('REDIS_CLUSTER', 'redis'),
        'prefix' => env('REDIS_PREFIX', Str::slug(env('APP_NAME', 'laravel'), '_').'_database_'),
        'serializer' => Redis::SERIALIZER_IGBINARY,
        'compression' => Redis::COMPRESSION_LZ4,
    ],

    'default' => [
        'host' => env('REDIS_DEFAULT_HOST', '127.0.0.1'),
        'password' => env('REDIS_DEFAULT_PASSWORD', null),
        'port' => env('REDIS_DEFAULT_PORT', '6379'),
        'database' => env('REDIS_DEFAULT_DB', '0'),
    ],

    'cache' => [
        'host' => env('REDIS_CACHE_HOST', '127.0.0.1'),
        'password' => env('REDIS_CACHE_PASSWORD', null),
        'port' => env('REDIS_CACHE_PORT', '6379'),
        'database' => env('REDIS_CACHE_DB', '0'),
    ],

    'queue' => [
        'host' => env('REDIS_QUEUE_HOST', '127.0.0.1'),
        'password' => env('REDIS_QUEUE_PASSWORD', null),
        'port' => env('REDIS_QUEUE_PORT', '6379'),
        'database' => env('REDIS_QUEUE_DB', '0'),
        'options'=> [ // Was not aware that it was possible to override the main options config in top like this. But it does and it solved my issue as I can disable serializer and compression only for my queue connection to redis.  All examples I could find online never mentioned options inside connection block.  But maybe it's common knowledge or I just missed it in docs.
            'serializer' => 0,
            'compression' => 0,
        ],
    ],

],`

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.

5 participants