-
Notifications
You must be signed in to change notification settings - Fork 939
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
fix: Make restore accept ttl in ms #1724
Conversation
Signed-off-by: Vladislav Oleshko <vlad@dragonflydb.io>
@@ -207,14 +202,11 @@ class RestoreArgs { | |||
|
|||
[[nodiscard]] bool RestoreArgs::UpdateExpiration(int64_t now_msec) { | |||
if (HasExpiration()) { | |||
auto new_ttl = CalculateExpirationTime(!abs_time_, abs_time_, expiration_, now_msec); | |||
if (new_ttl > kMaxExpireDeadlineSec * 1000) { | |||
int64_t ttl = abs_time_ ? expiration_ - now_msec : expiration_; |
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.
At some point we should really consider chrono
for anything related to time. expiration
could just be std::chrono::milliseconds
. That's a personal preference though, I am not advocating this heavily.
Moreover, consider making ttl
a const
. I am not advocating this heavily but my inner ocd for const everything is hard to ignore and not write a comment 👀
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.
Yes, same as with using mixed int/uint. Its a little mess but it's everywhere
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.
I don't see any reason making local variables in a 5 line function ever const, its visual clutter. What does it guard against? Accidentally assigning it and not the member variable?
expiration_ += now_msec; | ||
} | ||
|
||
expiration_ = ttl < 0 ? -1 : ttl + now_msec; |
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.
Is there a reason to set it to -1
and not leave it as is ? (again the semantics for expiration_
can be a expressed in a more modern way, but this is purely stylistic so there is no real value to discuss this here -- I just like to think out loud and that's why I am making this comment)
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.
No reason, It just explicitly underlines that negative values have only one purpose - mark an already expired state and the value doesn't matter
Restore accepts ttl in milliseconds https://redis.io/commands/restore/#:~:text=If%20ttl%20is%200%20the%20key%20is%20created%20without%20any%20expire%2C%20otherwise%20the%20specified%20expire%20time%20(in%20milliseconds)%20is%20set.