Visitar URL original
feat: Add optional GLIDE client for Redis online reads by chandlerok · Pull Request #6870 · feast-dev/feast · GitHub
Skip to content

feat: Add optional GLIDE client for Redis online reads #6870

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
ntkathole merged 2 commits into from
Oct 1, 2026
Merged

feat: Add optional GLIDE client for Redis online reads #6870

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8182dc4
feat: Add optional GLIDE client for Redis online reads
chandlerok Sep 25, 2026
6dd260a
fix: Release the GLIDE client and cache its module per store
chandlerok Sep 30, 2026
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
fix: Release the GLIDE client and cache its module per store 
Addresses review feedback on the GLIDE read path:

- teardown() now closes the native GLIDE client and clears the cached
  reference, so its Rust-side resources and file descriptors are released.
- The glide_sync module is imported once per store and cached on the
  instance instead of being looked up on every read.
- Allowlist the test password literals so detect-secrets stops failing
  lint-python.

Signed-off-by: Chandler King <chandleroking@gmail.com>
  • Loading branch information
@chandlerok @ntkathole
chandlerok authored and ntkathole committed Oct 1, 2026
commit 6dd260a67bacf42a3d21f87814bb6aeeda720cf9
18 changes: 15 additions & 3 deletions sdk/python/feast/infra/online_stores/redis.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -239,6 +239,7 @@ class RedisOnlineStore(OnlineStore):
# Only set when `client: glide` is configured, so the native extension stays
# untouched for users who never opt in.
_glide_client: Optional[Any] = None
_glide_sync: Optional[Any] = None

@ntkathole ntkathole Sep 26, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nit: _glide_client is cached here but never cleaned up. The existing teardown() closes the redis-py client — the GLIDE client should get the same treatment to release native (Rust-side) resources, file descriptors, and the connection pool.

Something like:

def teardown(self, config, tables, entities):
    ...
    if self._glide_client:
        self._glide_client.close()
        self._glide_client = None

@chandlerok chandlerok Sep 30, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done in d12d035. teardown() now closes self._glide_client and sets it back to None, so the native client is released as you suggested.

One note: the redis-py client was not actually being closed in teardown() before this change either (the method only deletes keys), so I scoped this to the GLIDE client rather than also changing the redis-py lifecycle in this PR. Happy to close that one too if you'd prefer it here.

@property
def async_supported(self) -> SupportedAsyncMethods:
Expand Down Expand Up @@ -350,6 +351,12 @@ def teardown(
for join_keys in join_keys_to_delete:
self.delete_entity_values(config, list(join_keys))

# Release the native GLIDE client, if one was ever opened, so teardown does
# not leave its Rust-side resources and file descriptors behind.
if self._glide_client:
self._glide_client.close()
self._glide_client = None

@staticmethod
def _parse_connection_string(connection_string: str):
"""
Expand Down Expand Up @@ -430,11 +437,17 @@ async def _get_client_async(self, online_store_config: RedisOnlineStoreConfig):
self._client_async = redis_asyncio.Redis(**kwargs)
return self._client_async

def _get_glide_sync(self):
"""Returns the cached ``glide_sync`` module, importing it on first use."""
if self._glide_sync is None:
self._glide_sync = _load_glide_sync()
return self._glide_sync

def _get_glide_client(self, online_store_config: RedisOnlineStoreConfig):
"""Creates and caches a GLIDE client built from the online store config."""
if not self._glide_client:
self._glide_client = _create_glide_client(
_load_glide_sync(), online_store_config
self._get_glide_sync(), online_store_config
)
return self._glide_client

Expand Down Expand Up @@ -712,9 +725,8 @@ def _read_hash_fields(
assert isinstance(online_store_config, RedisOnlineStoreConfig)

if online_store_config.client == RedisClient.glide:
glide_sync = _load_glide_sync()
return _glide_hmget_batch(

@ntkathole ntkathole Sep 26, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

_load_glide_sync() is called on every read when client == RedisClient.glide. While Python caches imported modules in sys.modules, this still incurs a function-call + dict-lookup overhead per read that is easy to avoid.

Consider caching the module reference on the instance alongside _glide_client, e.g.:

_glide_sync: Optional[Any] = None

def _get_glide_sync(self):
    if self._glide_sync is None:
        self._glide_sync = _load_glide_sync()
    return self._glide_sync

Then _read_hash_fields and _get_glide_client can both use self._get_glide_sync() instead of calling _load_glide_sync() each time.

@chandlerok chandlerok Sep 30, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done in d12d035. Added _get_glide_sync(), which imports the module once and caches it on the instance; both _read_hash_fields and _get_glide_client now go through it instead of calling _load_glide_sync() each read.

glide_sync,
self._get_glide_sync(),
self._get_glide_client(online_store_config),
commands,
)
Expand Down
46 changes: 44 additions & 2 deletions sdk/python/tests/unit/infra/online_store/test_redis.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -866,7 +866,7 @@ def test_glide_client_config_parses_connection_string():
]
assert config.database_id == 2
assert config.use_tls is True
assert config.credentials.password == "hunter2"
assert config.credentials.password == "hunter2" # pragma: allowlist secret
assert config.credentials.username == "feast"
# redis-py takes timeouts in seconds, GLIDE in milliseconds.
assert config.request_timeout == 1500
Expand Down Expand Up @@ -903,7 +903,7 @@ def test_glide_client_config_parses_cluster_connection_string():
("redis2", 6379),
]
assert config.use_tls is True
assert config.credentials.password == "hunter2"
assert config.credentials.password == "hunter2" # pragma: allowlist secret
# Cluster nodes do not support SELECT, so db is not forwarded.
assert config.database_id is None

Expand Down Expand Up @@ -934,3 +934,45 @@ def test_glide_client_config_warns_about_ignored_params(caplog):
)

assert "skip_full_coverage_check" in caplog.text


def test_glide_module_is_imported_once_per_store(
redis_online_store: RedisOnlineStore, feature_view
):
"""The glide_sync module is cached on the store, not re-imported per read."""
with (
patch(GLIDE_PATH, return_value=object()) as load_glide,
patch.object(redis_online_store, "_get_glide_client", return_value=object()),
patch(GLIDE_BATCH_PATH, MagicMock(return_value=[[None] * 4])),
):
for _ in range(2):
redis_online_store.online_read(
_online_read_config(RedisClient.glide),
feature_view,
_two_entity_keys()[:1],
["feature_10"],
)

load_glide.assert_called_once()


def test_teardown_closes_the_glide_client(
redis_online_store: RedisOnlineStore, repo_config: RepoConfig
):
"""teardown must release the native GLIDE client and clear the cache."""
glide_client = MagicMock()
redis_online_store._glide_client = glide_client

redis_online_store.teardown(repo_config, [], [])

glide_client.close.assert_called_once()
assert redis_online_store._glide_client is None


def test_teardown_without_a_glide_client_does_not_raise(
redis_online_store: RedisOnlineStore, repo_config: RepoConfig
):
"""A store that never opted into GLIDE must tear down cleanly."""
redis_online_store.teardown(repo_config, [], [])

assert redis_online_store._glide_client is None
Loading