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

removing trademarks from sentinel code #269

Open
daniel-house opened this issue Apr 8, 2024 · 3 comments
Open

removing trademarks from sentinel code #269

daniel-house opened this issue Apr 8, 2024 · 3 comments

Comments

@daniel-house
Copy link
Member

daniel-house commented Apr 8, 2024

I started on removing trademarks on the sentinel code (see #268 ) but it seems that both of the simple rules redis -> server and redis -> valkey might be rejected, so that I would have to do things twice or even three times. This issue is being opened so that we can discuss what you would like to see.

  • There may be objections to redis -> server on the grounds that this is in sentinel, not the server. redis -> sentinel seems better to me, but looks a bit strange in tls.c.

  • redis -> valkey seems to be legal and safe everywhere in sentinel.c and tls.c.

  • Sentinel uses code that also appears in both hiredis and the server. There are many structures and functions (e.g., redisAeAddRead) that are nearly identical in both sentinel.c and hiredis. For the objects that are static in sentinel.c, I prefer redis -> sentinel.

  • I would rename sentinelRedisInstance to sentinelValkeyInstance and rename the SRI abbreviation to SVI.

@zuiderkwast
Copy link
Contributor

  • Sentinel uses code that also appears in both hiredis and the server. There are many structures and functions (e.g., redisAeAddRead) that are nearly identical in both sentinel.c and hiredis. For the objects that are static in sentinel.c, I prefer redis -> sentinel.
#include "hiredis.h"

It's not "nearly identical". It is the hiredis functions. Sentinel is using hiredis. Since we have decided to not change deps/hiredis code, we shall also keep the calls to these functions. (If we later decide to replace hiredis with a fork of hiredis, we can change all of this, but not now.)

  • I would rename sentinelRedisInstance to sentinelValkeyInstance and rename the SRI abbreviation to SVI.

OK, sounds good.

@daniel-house
Copy link
Member Author

daniel-house commented Apr 11, 2024

The #include "hiredis.h" seems to be suggesting that I delete the functions in sentinel.c that are identical with the functions in deps/hiredis/adapters/ae.h and rely on #include "hiredis.h" to replace them.

Is that what you intended?

If so, please consider this comment found in sentinel.c:

/* ======================= hiredis ae.c adapters =============================
 * Note: this implementation is taken from hiredis/adapters/ae.h, however
 * we have our modified copy for Sentinel in order to use our allocator
 * and to have full control over how the adapter works. */

Also, these functions are all static in both deps/hiredis/adapters/ae.h and in sentinel.c, so they cannot be easily reached. Perhaps I could follow deps/hiredis/examples/example-ae.c but I have to ask why antirez didn't do that.

@zuiderkwast
Copy link
Contributor

Wow, this is more complicated than I thought. 😁

I suggest you just leave them as redis. Then the comment you quoted will still be correct.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants