Closed vstrimaitis closed 2 years ago
hey @vstrimaitis thanks for reporting this issue and for the detailed investigation! we'll take a look at this ASAP
Looks like this is still broken since we rolled back the PR due to some issues with the new version of redis clusters (see bug on redis-py: https://github.com/redis/redis-py/issues/2003).
Might need to roll back to redis-py-cluster to make this work properly
Confirming that I'm also seeing this issue with Feast=0.18.1.
I noticed that I get the following version compatibility warning.
redis-py-cluster 2.1.3 has requirement redis<4.0.0,>=3.0.0, but you'll have redis 4.1.4 which is incompatible.
Sharing in case it's helpful in any way.
See https://github.com/feast-dev/feast/pull/2347 for a rollback PR
I solved this issue,here is my code from redis.cluster import RedisCluster,ClusterNode startup_nodes =[] for node in nodes: c = ClusterNode(host=node['host'],port=node['port']) startup_nodes.append(c) redis_client = RedisCluster(startup_nodes=startup_nodes, password= password,decode_responses=True) Please pay attention to the source code. The parameter startup_nodes in RedisCluster must be the ClusterNode class.
Expected Behavior
Using a Redis cluster as the online storage should work without errors.
Current Behavior
Using a Redis cluster as the online storage no longer works after updating from 0.17.0 to 0.18.0. The issue happens whenever Feast tries to connect to the Redis cluster - e.g. during
feast materialize
or during feature serving. The exact error that is raised looks like this:Steps to reproduce
feast materialize
Specifications
Possible Solution
Some investigation revealed that versions 0.17.0 and 0.18.0 differ in the sense that 0.18.0 uses
RedisCluster
fromredis.cluster
instead ofrediscluster
. As a result, the code here breaks because the newRedisCluster
instance expects to receive a list ofClusterNode
instead of a list of dicts. Temporarily we worked around this issue by creating a custom online store (by copying all of the code in the linked file 😁 ) and changing it like so:This worked for us, but I'm not sure if this is the best approach in general because I'm not too familiar with the codebase yet.