removing trademarks from sentinel code

valkey-io / valkey

A flexible distributed key-value datastore that supports both caching and beyond caching workloads.

https://valkey.io

Other

14.78k stars 524 forks source link

removing trademarks from sentinel code #269

Open daniel-house opened 3 months ago

daniel-house commented 3 months ago

I started on removing trademarks on the sentinel code (see https://github.com/valkey-io/valkey/pull/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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

Wow, this is more complicated than I thought. :grin:

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