Adding resiliency in RedisClusterClient #44
Conversation
andrijaperovic
force-pushed
the
master
branch
from
29b868a
to
a9fc4ad
Compare
| bootstrap.option(EpollChannelOption.TCP_KEEPINTVL, 5); | ||
| bootstrap.option(EpollChannelOption.TCP_KEEPCNT, 3); | ||
| // Socket Timeout (milliseconds) | ||
| bootstrap.option(EpollChannelOption.TCP_USER_TIMEOUT, 60000); |
There was a problem hiding this comment.
thoughts on having these be exposed in the cluster config?
There was a problem hiding this comment.
I think that makes sense. Also, these are very cloud provider specific for Azure Cache for Redis in terms of how socket timeouts are handled (redis/lettuce#1428), so I'm wondering if it makes sense to subclass RedisClusterClient and make it cloud-provider specific (e.g. AzureRedisClusterClient, AwsRedisClusterClient, GcpRedisClusterClient, etc.).
There was a problem hiding this comment.
Cloud provider can be set via config property (e.g. cloud_provider: azure).
There was a problem hiding this comment.
Do you have a usecase for a stateful redis connection in newer versions of feast?
EDIT:
Looks like it - https://github.com/feast-dev/feast/blob/a7d4cb71af97156905e4567e3ccf484f8255ca88/java/storage/connectors/redis/src/main/java/feast/storage/connectors/redis/retriever/RedisClusterClient.java#L22
There was a problem hiding this comment.
i.e. this wouldnt hold true if you wanted to run your own Redis cluster in Azure?
Might make sense then to instead allow for additional azure_configs within the RedisClusterConfig?
There was a problem hiding this comment.
@adchia correct, if we ran something like redis-server instead of the managed service we would not need the custom socket timeout configuration and tcp keep-idle config. I suspect that it would also not be required in the MemoryDB/ElastiCache and Memorystore managed Redis scenarios.
(handle tcp keepidle in failover scenario, auth with ssl & handle cluster details endpoint returning IP in ssl scenario. Signed-off-by: Andrija Perovic <andrija.perovic@sap.com>
andrijaperovic
force-pushed
the
master
branch
from
31c4ead
to
4e16577
Compare
Signed-off-by: Andrija Perovic <andrija.perovic@sap.com>
Signed-off-by: Andrija Perovic <andrija.perovic@sap.com>
andrijaperovic
force-pushed
the
master
branch
from
f80c9fc
to
af4f53d
Compare
Signed-off-by: Andrija Perovic <andrija.perovic@sap.com>
…vel. Signed-off-by: Andrija Perovic <andrija.perovic@sap.com>
…vel. Signed-off-by: Andrija Perovic <andrija.perovic@sap.com>
…vel. Signed-off-by: Andrija Perovic <andrija.perovic@sap.com>
Signed-off-by: Andrija Perovic <andrija.perovic@sap.com>
Signed-off-by: Andrija Perovic <andrija.perovic@sap.com>
@snowmanmsft @adchia