Remove call to TIME in Lua scripts #11
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Type: Enhancement
PR Summary: This pull request introduces an enhancement to the token bucket implementation by removing the direct call to the Redis TIME command within Lua scripts. Instead, it adds a new function to generate a time tuple in Python, which is then passed as arguments to the Lua script. This change addresses compatibility issues with certain Redis deployments, such as AWS ElasticCache Serverless Redis, by ensuring that the time is determined outside of the Lua script and passed in, thus avoiding the non-deterministic behavior of the TIME command.
Decision: Comment
📝 Type: 'Enhancement' - not supported yet.
- Sourcery currently only approves 'Typo fix' PRs.
✅ Issue addressed: this change correctly addresses the issue or implements the desired feature.
No details provided.
📝 Complexity: the changes are too large or complex for Sourcery to approve.
- Unsupported files: the diff contains files that Sourcery does not currently support during reviews.
General suggestions:
- Ensure that the new method of passing time as arguments does not introduce any precision loss or timing issues, especially in high-load environments.
- Consider adding a fallback mechanism or a configuration option to use the original TIME command approach for deployments where it is supported and preferred.
- Review the overall impact of this change on the performance of the token bucket mechanism, particularly in terms of the additional overhead of calculating the time in Python and passing it to Redis.
Thanks for using Sourcery. We offer it for free for open source projects and would be very grateful if you could help us grow. If you like it, would you consider sharing Sourcery on your favourite social media? ✨
klette
force-pushed
the
deterministic-lua-scripts
branch
2 times, most recently
from
7e43605
to
56a4ba0
Compare
TIME is not deterministic and is rejected in some Redis deployments, such as AWS ElasticCache Servless Redis. Rather than calling TIME within the lua script, we pass the information as arguments.
klette
force-pushed
the
deterministic-lua-scripts
branch
from
56a4ba0
to
af18fd5
Compare
TIME is not deterministic and is rejected in some Redis deployments, such as AWS ElasticCache Servless Redis.
We pass the time tuple as arguments rather than calling TIME within the Lua script.