Skip to content

Add support for Redis and Redis Cluster #1511

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.

Sign up for GitHub

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

Merged
feast-ci-bot merged 22 commits into from
Jun 9, 2021
Merged

Add support for Redis and Redis Cluster #1511

Changes from 1 commit
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
d97aaea
Add support for Redis as online store
qooba Apr 25, 2021
ae9ed8e
Add support for Redis as online store
qooba Apr 25, 2021
e165f00
Add redis provider
qooba Apr 26, 2021
df8f3cb
Add redis online provider
qooba Apr 27, 2021
6bea80d
Add redis online provider
qooba Apr 27, 2021
5745ff7
Add redis online provider
qooba Apr 27, 2021
a3c4f3b
Add redis online provider
qooba Apr 28, 2021
e434e59
Add redis online provider - integration tests
qooba May 6, 2021
8ccfc6b
Add redis online provider - integration tests
qooba May 6, 2021
be9a17f
Add redis provider
qooba May 12, 2021
7606fe7
Add redis provider
qooba May 12, 2021
2a95e3e
correct redis provider
qooba May 24, 2021
435c856
correct redis provider
qooba May 26, 2021
d329439
correct redis provider test
qooba May 26, 2021
b5269e7
correct redis provider
qooba May 30, 2021
f602b9a
correct redis provider
qooba May 30, 2021
bbde0b0
change redis connection string
qooba Jun 2, 2021
d86f23c
change redis connection string
qooba Jun 2, 2021
b0c8101
fix redis materialize
qooba Jun 7, 2021
52232cc
Rename connection string and set defaults
woop Jun 8, 2021
53f8f72
Fix argument in redis test
woop Jun 9, 2021
e12ecff
Fix broken connection string
woop Jun 9, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Add redis provider 
Signed-off-by: qooba <dev@qooba.net>
  • Loading branch information
@qooba @woop
qooba authored and woop committed Jun 9, 2021
commit e165f003a81ad386359626c2e5a5d55602befd60
56 changes: 43 additions & 13 deletions sdk/python/feast/infra/redis_provider.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import json
Copy link
Copy Markdown
Member

@woop woop May 25, 2021

Choose a reason for hiding this comment

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

redis.py for the file name is fine, for consistency.

import os
from datetime import datetime
from pathlib import Path
Expand Down Expand Up @@ -32,18 +33,6 @@ class RedisProvider(Provider):
def __init__(self, config: RepoConfig):
assert isinstance(config.online_store, RedisOnlineStoreConfig)

def _get_client(self):
if os.environ["REDIS_TYPE"] == "REDIS_CLUSTER":
return RedisCluster(
host=os.environ["REDIS_HOST"],
port=os.environ["REDIS_PORT"],
decode_responses=True,
)
else:
return Redis(
host=os.environ["REDIS_HOST"], port=os.environ["REDIS_PORT"], db=0
)

def update_infra(
self,
project: str,
Expand All @@ -54,7 +43,6 @@ def update_infra(
partial: bool,
):
client = self._get_client()
# TODO

def teardown_infra(
self,
Expand Down Expand Up @@ -178,6 +166,48 @@ def materialize_single_feature_view(
feature_view.materialization_intervals.append((start_date, end_date))
registry.apply_feature_view(feature_view, project)

def _get_cs(self):
"""
Reads Redis connections string using format
for RedisCluster:
redis1:6379,redis2:6379,decode_responses=true,skip_full_coverage_check=true,ssl=true,password=...
for Redis:
redis_master:6379,db=0,ssl=true,password=...
"""
connection_string = os.environ["REDIS_CONNECTION_STRING"]
startup_nodes = [
dict(zip(["host", "port"], c.split(":")))
for c in connection_string.split(",")
if not "=" in c
]
params = {}
for c in connection_string.split(","):
if "=" in c:
kv = c.split("=")
try:
kv[1] = json.loads(kv[1])
except json.JSONDecodeError:
...

it = iter(kv)
params.update(dict(zip(it, it)))

return startup_nodes, params

def _get_client(self):
"""
Creates the Redis client RedisCluster or Redis depending on configuration
"""
startup_nodes, kwargs = self._get_cs()

if os.environ["REDIS_TYPE"] == "REDIS_CLUSTER":
Copy link
Copy Markdown
Member

@woop woop May 24, 2021

Choose a reason for hiding this comment

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

We certainly should be moving to using purely feature_store.yaml for configuration, and not environmental variables. It will be too hard to keep track of these configuration backdoors.

Copy link
Copy Markdown

@abrookins abrookins May 24, 2021

Choose a reason for hiding this comment

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

Does feature_store.yaml support provider-specific configuration? If it does, I agree that using it may be better than environment variables, except the connection string, of course. I assume the project YAML is supposed to be stored with the project in git, etc., so you wouldn't add connection details there?

Copy link
Copy Markdown
Member

@woop woop May 24, 2021

Choose a reason for hiding this comment

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

Yes, that is the goal. Also, I think we can add environmental variable loading when parsing the feature_store.yaml file, or optionally make it possible to set values inside the file using env vars. That way we are keeping track of all options in a single place, and loading of env vars happens during initialization, not in a place deeply nested in the code.

Copy link
Copy Markdown

@abrookins abrookins May 25, 2021

Choose a reason for hiding this comment

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

Ah, that's a good idea! I agree that it's best to have a single place that all of the configuration options are visible.

kwargs["startup_nodes"] = startup_nodes
return RedisCluster(**kwargs)
else:
kwargs["host"] = startup_nodes[0]["host"]
kwargs["port"] = startup_nodes[0]["port"]
return Redis(**kwargs)

@staticmethod
def get_historical_features(
config: RepoConfig,
Expand Down