Fix time parsing for limit cache #45
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Owner
|
Thanks @flooey for the fix. Can you update the format from If you make those 2 minor changes, I will merge. Thanks again! |
Fix a bug in the code that handles determining the timestamp for
values in the cache that's used to implement removing the oldest key
when the limit is reached. The bug is in parsing the time returned by
Redis, making it produce incorrect results when the leading portion of
the microseconds moves from 09 to 10. This tended to reveal itself as
flakiness in the `test_limit` unit test, which we were seeing around 1
in 200 runs.
The `TIME` operation in Redis returns the time as an array of two
strings, the first holding the number of seconds and the second
holding the number of microseconds. Importantly, the second string
has no leading zeroes. The Lua code parses this by concatenating
these strings together separated by a decimal point and then parsing
that as a number, which produces incorrectly ordered results when
faced with values like `("1234567", "999")` and `("1234567", "1000")`,
parsing them as 1234567.999000 and 1234567.100000, respectively,
meaning the second value will be expunged from the cache first despite
being inserted second.
Instead, parse each returned string as a number and then combine them
numerically, which produces the correct number in this situation.
I can't easily produce a test for this, because to test this
deterministically you have to control the timestamps Redis produces,
and the current testing setup uses a standalone Redis process. We
have a internal unit test that uses `freezegun` and an in-process
Redis fake to test it, but I didn't think you would want that level of
change to the testing configuration just to make the test for this
very narrow change work.
Contributor
Author
Sure thing, I've added a comment. Unfortunately, the |
Contributor
Author
|
Just checking in on this. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fix a bug in the code that handles determining the timestamp for values in the cache that's used to implement removing the oldest key when the limit is reached. The bug is in parsing the time returned by Redis, making it produce incorrect results when the leading portion of the microseconds moves from 09 to 10. This tended to reveal itself as flakiness in the
test_limitunit test, which we were seeing around 1 in 200 runs.The
TIMEoperation in Redis returns the time as an array of two strings, the first holding the number of seconds and the second holding the number of microseconds. Importantly, the second string has no leading zeroes. The Lua code parses this by concatenating these strings together separated by a decimal point and then parsing that as a number, which produces incorrectly ordered results when faced with values like("1234567", "999")and("1234567", "1000"), parsing them as 1234567.999000 and 1234567.100000, respectively, meaning the second value will be expunged from the cache first despite being inserted second.Instead, parse each returned string as a number and then combine them numerically, which produces the correct number in this situation.
I can't easily produce a test for this, because to test this deterministically you have to control the timestamps Redis produces, and the current testing setup uses a standalone Redis process. We have a internal unit test that uses
freezegunand an in-process Redis fake to test it, but I didn't think you would want that level of change to the testing configuration just to make the test for this very narrow change work.