Skip to content

Register abs-expire module APIs#11025

Merged
oranagra merged 1 commit intoredis:unstablefrom
chenyang8094:register-abs-expire
Jul 24, 2022
Merged

Register abs-expire module APIs#11025
oranagra merged 1 commit intoredis:unstablefrom
chenyang8094:register-abs-expire

Conversation

@chenyang8094
Copy link
Contributor

@chenyang8094 chenyang8094 commented Jul 22, 2022

RM_SetAbsExpire and RM_GetAbsExpire were not actually operational since they were introduced, due to omission in API registration.
Sorry this was an omission in the previous PR #8564

@enjoy-binbin
Copy link
Contributor

i guess it also need to backport to 6.2

@oranagra
Copy link
Member

seen this kind of error a few times before..
the downside of not adding a test even for the trivial APIs.
or the fact that there's too many things to do in order to add one.
maybe we should add an assertion in RedisModule_Init when RedisModule_GetApi returns NULL?

@oranagra oranagra merged commit 39d216a into redis:unstable Jul 24, 2022
@oranagra oranagra added the release-notes indication that this issue needs to be mentioned in the release notes label Jul 24, 2022
@enjoy-binbin
Copy link
Contributor

enjoy-binbin commented Jul 24, 2022

maybe we should add an assertion in RedisModule_Init when RedisModule_GetApi returns NULL?

yes, i think we can do it, like assert(REDISMODULE_GET_API(SetAbsExpire) == REDISMODULE_OK);

i guess we need it, i did it and i found that we have an extra ReplySetPushLength (Looks like dead code)
in here: https://github.com/redis/redis/pull/8521/files#diff-26821aebe5bf67b83e43e39c63ad877ff6caa7b023e88712d5ab28463974542aR907

Fortunately nothing else was missed, but doing this require a lot of changes...

@oranagra
Copy link
Member

the assertion can be part of the macro.

enjoy-binbin added a commit to enjoy-binbin/redis that referenced this pull request Jul 24, 2022
Prevent forgetting to register API, adding assertions, see redis#11025
Also remove `ReplySetPushLength`, it is a dead code.
@enjoy-binbin
Copy link
Contributor

ok, i am doing it, in #11033

@oranagra oranagra changed the title Register abs-expire apis Register abs-expire module APSs Aug 22, 2022
@oranagra oranagra changed the title Register abs-expire module APSs Register abs-expire module APIs Aug 22, 2022
@oranagra oranagra mentioned this pull request Sep 21, 2022
oranagra pushed a commit that referenced this pull request Sep 21, 2022
RM_SetAbsExpire and RM_GetAbsExpire were not actually operational since
they were introduced, due to omission in API registration.

(cherry picked from commit 39d216a)
@oranagra oranagra mentioned this pull request Dec 11, 2022
oranagra pushed a commit that referenced this pull request Dec 12, 2022
RM_SetAbsExpire and RM_GetAbsExpire were not actually operational since
they were introduced, due to omission in API registration.

(cherry picked from commit 39d216a)
enjoy-binbin pushed a commit to enjoy-binbin/redis that referenced this pull request Jul 31, 2023
RM_SetAbsExpire and RM_GetAbsExpire were not actually operational since
they were introduced, due to omission in API registration.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release-notes indication that this issue needs to be mentioned in the release notes

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants