Skip to content
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

Add GetAbsExpire/SetAbsExpire for module #8564

Merged
merged 1 commit into from
Mar 24, 2021
Merged

Add GetAbsExpire/SetAbsExpire for module #8564

merged 1 commit into from
Mar 24, 2021

Conversation

chenyang8094
Copy link
Collaborator

@chenyang8094 chenyang8094 commented Feb 26, 2021

Similar to the SET key value [PXAT <milliseconds-timestamp>], it is often necessary to set and get the absolute expire time of the key in the module. Otherwise I can only do it by this:

long long milliseconds = expire - RedisModule_Milliseconds();  
RedisModule_SetExpire(key, milliseconds);

Now i can do it by this:

RedisModule_SetAbsExpire(key, expire);

At the same time, in the previous implementation of RM_SetExpire, expire is not forced to be a positive value, so it is possible to get a value that conflicts with REDISMODULE_NO_EXPIRE(-1) in RM_GetExpire, so I Add a check to ensure that the expire parameters in RM_SetExpire and RM_SetAbsExpire must be positive.

@soloestoy soloestoy added the state:major-decision Requires core team consensus label Mar 10, 2021
@soloestoy
Copy link
Collaborator

I think the new APIs are useful, ping @redis/core-team get more suggestions.

oranagra
oranagra previously approved these changes Mar 10, 2021
Copy link
Member

@oranagra oranagra left a comment

Choose a reason for hiding this comment

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

i agree with the usefulness of this API.
i have some concern about the details, but on further thinking i think the current version may be ok.

src/module.c Outdated Show resolved Hide resolved
@oranagra oranagra added release-notes indication that this issue needs to be mentioned in the release notes approval-needed Waiting for core team approval to be merged labels Mar 10, 2021
oranagra
oranagra previously approved these changes Mar 18, 2021
itamarhaber
itamarhaber previously approved these changes Mar 18, 2021
yossigo
yossigo previously approved these changes Mar 18, 2021
src/module.c Outdated Show resolved Hide resolved
Add a check to ensure that the expire parameters in RM_SetExpire and RM_SetAbsExpire must be positive.
src/module.c Show resolved Hide resolved
@oranagra oranagra added the state:to-be-merged The PR should be merged soon, even if not yet ready, this is used so that it won't be forgotten label Mar 21, 2021
@oranagra oranagra merged commit ccc39f6 into redis:unstable Mar 24, 2021
@oranagra oranagra mentioned this pull request Apr 19, 2021
JackieXie168 pushed a commit to JackieXie168/redis that referenced this pull request Sep 8, 2021
Add a check to ensure that the expire parameters in RM_SetExpire and RM_SetAbsExpire must be positive.
@oranagra oranagra mentioned this pull request Sep 21, 2022
@oranagra oranagra mentioned this pull request Dec 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approval-needed Waiting for core team approval to be merged release-notes indication that this issue needs to be mentioned in the release notes state:major-decision Requires core team consensus state:to-be-merged The PR should be merged soon, even if not yet ready, this is used so that it won't be forgotten
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants