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

Fix RedisModule_IsAOFClient Redis Module API #8596

Merged

Conversation

YaacovHazan
Copy link
Collaborator

@YaacovHazan YaacovHazan commented Mar 3, 2021

Since the API declared (as #define) in redismodule.h and uses
the CLIENT_ID_AOF that declared in the server.h, when
a module will want to make use of this API, it will get a compilation
error (module doesn't include the server.h).

The API was broken by d6eb3af (failed attempt for a cleanup).
Revert to the original version of RedisModule_IsAOFClient
that uses UINT64_MAX instead of CLIENT_ID_AOF

@oranagra
Copy link
Member

oranagra commented Mar 4, 2021

looking back at the history, i see that the original commit that added this API actually did work: 97f6e31
but it was later broken by d6eb3af
i see that the commit that broke it only exists in 6.2, so if we want to keep being compatible with pre-6.2 modules that might have used it, we need to just revert that one line in redismodule.h.
note that modules that were compiled against and old header file currently still work with redis 6.2, is just that they'll fail to build against a 6.2 header file.
@guybe7 is there any reason not to revert that change you did in redismodule.h? the commit comment doesn't explain it, so i suppose it was just a failed attempt for a cleanup.

@oranagra oranagra added this to To do in 6.2 via automation Mar 4, 2021
@oranagra oranagra added the release-notes indication that this issue needs to be mentioned in the release notes label Mar 4, 2021
@guybe7
Copy link
Collaborator

guybe7 commented Mar 4, 2021

Yes, a failed cleanup attempt :(
But TBH I prefer what Yaacov did

@oranagra
Copy link
Member

oranagra commented Mar 4, 2021

i don't think we can afford it due to backward forward compatibility.

Since the API declared (as #define) in redismodule.h and uses
the CLIENT_ID_AOF that declared in the server.h, when
a module will want to make use of this API, it will get a compilation
error (module doesn't include the server.h).

The API was broken by d6eb3af (failed attempt for a cleanup).
Revert to the original version of RedisModule_IsAOFClient
that uses UINT64_MAX instead of CLIENT_ID_AOF
@YaacovHazan YaacovHazan force-pushed the fix-RedisModule_IsAOFClient-API branch from 053087d to 63cdc8b Compare March 4, 2021 10:59
@oranagra oranagra merged commit ac91822 into redis:unstable Mar 4, 2021
@oranagra oranagra moved this from To do to Done in 6.2 Mar 17, 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
Since the API declared (as #define) in redismodule.h and uses
the CLIENT_ID_AOF that declared in the server.h, when
a module will want to make use of this API, it will get a compilation
error (module doesn't include the server.h).

The API was broken by d6eb3af (failed attempt for a cleanup).
Revert to the original version of RedisModule_IsAOFClient
that uses UINT64_MAX instead of CLIENT_ID_AOF
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
No open projects
6.2
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants