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

redis cluster not supported for ingesting data #327

Closed
mihaileu opened this issue Mar 11, 2024 · 7 comments
Closed

redis cluster not supported for ingesting data #327

mihaileu opened this issue Mar 11, 2024 · 7 comments
Assignees

Comments

@mihaileu
Copy link

Pulse Version

1.0.0-beta14

Laravel Version

10.47.0

PHP Version

8.3

Livewire Version

3.4.8

Database Driver & Version

No response

Description

Lalavel pulse doesn't accept redis cluster settings as ingest driver

{"message":"Laravel\\Pulse\\Support\\RedisAdapter::client(): Return value must be of type Redis|Predis\\Client|Predis\\Pipeline\\Pipeline, RedisCluster returned","context":{"exception":{"class":"TypeError","message":"Laravel\\Pulse\\Support\\RedisAdapter::client(): Return value must be of type Redis|Predis\\Client|Predis\\Pipeline\\Pipeline, RedisCluster returned","code":0,"file":"/var/www/genius/vendor/laravel/pulse/src/Support/RedisAdapter.php:144"}},"level":400,"level_name":"ERROR","channel":"production","datetime":"2024-03-11T12:06:12.701186+02:00","extra":{}}

Steps To Reproduce

set laravel with a redis cluster connection https://laravel.com/docs/10.x/redis#clusters

use redis for ingesting data in pulse

@jessarcher
Copy link
Member

Hi @mihaileu,

Are you using client-side sharding or native Redis clustering? I'm having trouble setting up either locally, so I'm figuring out where to focus my efforts.

@mihaileu
Copy link
Author

mihaileu commented Mar 13, 2024

Redis clustering, using aws elasti cache redis cluster.
These are the settings:

   'client' => env('REDIS_CLIENT', 'phpredis'),
 
    'options' => [
       'cluster' => env('REDIS_CLUSTER', 'redis'),
       'failover' => 2,
       'clusters' => [
           'default' => [
              [
               //connection details
              ]
           ] 
       ]
    ],

@jessarcher
Copy link
Member

Hi @mihaileu,

I've replicated the issue. The type error is easy enough to fix, however, I'm having trouble figuring out how to run the commands correctly and against which target node(s).

I'm also struggling to set up a cluster in GitHub Actions so any changes can be tested.

I've never used a Redis cluster before, so I'm in the dark here. Do you have any ideas what would be required, or can you submit a PR?

Copy link

Thank you for reporting this issue!

As Laravel is an open source project, we rely on the community to help us diagnose and fix issues as it is not possible to research and fix every issue reported to us via GitHub.

If possible, please make a pull request fixing the issue you have described, along with corresponding tests. All pull requests are promptly reviewed by the Laravel team.

Thank you!

@jessarcher
Copy link
Member

I got a cluster running in GitHub Actions so I've pushed my WIP at #335 to highlight the various errors.

It looks like Predis doesn't support stream commands like XADD with clusters, so that's probably a dead end.

PhpRedis looks a bit more promising, but there are two issues:

  1. There's an issue with pipelining. Not sure if this is a client limitation or something that can be solved.

  2. The stream commands expect a node key to be passed as the first argument, and I have no idea what that should be. The symptom of this is errors like this, where the command (e.g. XTRIM is being interpreted as the node key, which shuffles all the other arguments):

    Error running command [XTRIM laravel_database_laravel:pulse:ingest MINID ~ 946177445000]. Redis error: [ERR unknown command 'laravel_database_laravel:pulse:ingest', with args beginning with: 'MINID' '~' '946177445000' ].
    

@princejohnsantillan
Copy link

I'm seeing similar issues with a Laravel Vapor setup. Using the Redis 6.x Cluster. Temporarily using storage for PULSE_INGEST_DRIVER instead.

@driesvints
Copy link
Member

Thanks all. For now, we're going to close this as there's no high-demand for this feature. We'll leave Jess' WIP PR open for now.

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

No branches or pull requests

5 participants