-
Notifications
You must be signed in to change notification settings - Fork 60
fix #417: fix setting ttl on async pipeline #420
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
base: main
Are you sure you want to change the base?
Conversation
|
Hi, I’m Jit, a friendly security platform designed to help developers build secure applications from day zero with an MVS (Minimal viable security) mindset. In case there are security findings, they will be communicated to you as a comment inside the PR. Hope you’ll enjoy using Jit. Questions? Comments? Want to learn more? Get in touch with us. |
BEFORE: - await pipe.expire() triggers ClusterPipeline.__await__ - this triggers .initialize() which delete the pipeline commands AFTER: - pipe.expire() in case of pipeline - await pipe.expire() in case of classic client
e9db5d3 to
afbed2b
Compare
|
Thanks @dfroger! We'll take a look ASAP. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes an issue where setting TTL on async pipelines incorrectly triggered ClusterPipeline.__await__, which would prematurely initialize the pipeline and delete queued commands. The fix introduces a new _aexpire helper method that conditionally awaits based on whether the client is a pipeline or a regular async client.
Key Changes
- Added
_aexpirestatic method to handle TTL operations on async clients and pipelines - Updated
awritemethod to use the new_aexpirehelper instead of directly awaitingpipe.expire() - Added comprehensive test coverage for TTL functionality across sync, async, and cluster configurations
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| redisvl/index/storage.py | Added _aexpire helper method and updated awrite to use it; fixed documentation type hints |
| tests/integration/test_search_index.py | Added test for TTL functionality with sync client |
| tests/integration/test_cluster_pipelining.py | Added test for TTL functionality with cluster client |
| tests/integration/test_async_search_index.py | Added test for TTL functionality with async client |
Comments suppressed due to low confidence (2)
redisvl/index/storage.py:700
- The docstring parameter type 'AsyncClientOrPipeline' is inconsistent with the actual parameter type 'AsyncRedisClientOrPipeline' and inconsistent with the updated documentation in HashStorage._aset and HashStorage._aget. This should be changed to 'AsyncRedisClientOrPipeline' for consistency.
async def _aset(client: AsyncRedisClientOrPipeline, key: str, obj: Dict[str, Any]):
"""Asynchronously set a JSON obj in Redis for the given key.
Args:
client (AsyncClientOrPipeline): The async Redis client or pipeline instance.
redisvl/index/storage.py:727
- The docstring parameter type 'AsyncRedisClient' is inconsistent with the actual parameter type 'AsyncRedisClientOrPipeline' and inconsistent with the updated documentation in HashStorage._aset and HashStorage._aget. This should be changed to 'AsyncRedisClientOrPipeline' for consistency.
async def _aget(client: AsyncRedisClientOrPipeline, key: str) -> Dict[str, Any]:
"""Asynchronously retrieve a JSON object from Redis for the given key.
Args:
client (AsyncRedisClient): The async Redis client or pipeline instance.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
BEFORE:
await pipe.expire()triggersClusterPipeline.__await__.initialize()which delete the pipeline commandsAFTER:
pipe.expire()in case of pipelineawait pipe.expire()in case of classic clientHere is a first try to fix #417 .
Tests and lint are ok. However I failed to run
tests/integration/test_cluster_pipelining.py, cluster doesn't seems to start (test.log).