I wanted to start the conversation about trying to write more decoupled and more modular code in Redis. I want to start with two suggestions:
The first is that we should try to split up some of the large filesso that they are a little bit more modular and provide a coherent interface. I think the main argument against this is that it would obfuscate code history and complicate cherrypicks (since changes would be in new files), but I think in the long term it would be wroth it. Three specific files that come to mind are server.c/h, networking.c, and cluster.c/h. We could split them up maybe like this:
server.c/h -> Core redis server executable. This is the integration point of all the other files. Has a lot of glue code like starting up the event loop and running the cron, but has very little other code. command.c/h -> Definition for all of the Redis commands. info.c/h -> Implements the info/info like commands clients.c/h -> Implements the client interface and client structure. db.c/db.h -> Implements the database structure.
cluster.c/h -> Integration point for all cluster code, similar to server.c/h but for the cluster code. cluster_query/h -> Implements all of the routing code that handles the various types of moved/routing messages that are returned. cluster_networking.c/h -> implements the clusterbus/nodes/networking layer cluster_failover.c/h -> Implents the cluster failover command onto of the cluster networking primitives. migrate.c/h -> Implements the code for the migrate/dump/restore command.
The second suggestion is moving more towards opaque structures, and doing more proper constructing destructing of objects. I remember I missed some place where a client was being manually allocated because it didn't go through createClient.
I think this will help us maintain the codebase longterm, and doing this type of refactoring for 7.0 is probably a good idea, since we probably won't be back porting a lot of stuff after that.
Comment From: oranagra
I agree we'll need to do some of that sooner or later. My concern wouldn't be just the effect on backport cherry-picking, but also on merges to the many branches and forks redis has, and on top of that the damage to the blame log, which makes it hard to track down the origin and intent or history of some line.
These are obviously not a reason not to do such cleanup one day, we just need to pick the right day, and weigh the gains vs the loses. i.e. in the long run, i'll personally lose a lot of time as a consequence of such changes. i remember how much i suffered from the renaming of redisLog to serverLog and other disqueue related refactorings that were done at the time. The gains are obvious, safer, more maintainable and readable code, so i just wanna be sure we're aware of the loses and weigh them, not just do some cleanup because we can.
So other than weighing the gains and loses, and picking the right time, we also need to make sure we do it only once. i.e. it can be incremental, handling a different part in each time, but we should make sure we know exactly where we're aiming for so that we don't change the same part over and over.
FYI: @ushachar started working in that direction in #9080
Comment From: madolson
Agree with what you said (it will also cause much pain for ElastiCache as well). I assumed @ushachar was from RL and so just wanted to make sure we all agreed on the direction.
I think the incremental approach is fair, we can refactor bits of code piece by piece. We should just make sure the people writing the code converge to the interfaces we want longterm.
Comment From: yossigo
I agree with everything said above.
I think the urgency and value of restructuring varies greatly between different areas of the code base. For example, server.[ch] is pretty messy but it's mostly aesthetically unpleasing. On the other hand, replication.[ch] is in a state that's terrible to work with - it doesn't even clearly distinguish replica-side from master-side code.
Another example is client.[ch] where creating a more abstract interface is a prerequisite to any attempt to really solve the I/O threads issues.
As @oranagra says, we need to have a clear end goal and move there incrementally but linearly. To be able to do that we probably need to pick up a few cases, draft PRs, discuss them in detail and derive the more generalized patterns we want to adopt. #9080 is a good start.
Comment From: ushachar
Excellent - this is exactly the discussion I wanted to kick off with #9080 (Thanks @madolson!) I would add two more possible goals: - Move large, distinct functionality into subfolders (for example - /src/hashes/ containing crc & sha) - Split the refactored module header files into private/public components (.h and .i) to improve modularization and help understand the potential impact of changes
Comment From: ushachar
If everyone is in agreement about the direction I'll create a sample PR addressing the crc|sha code (probably the easiest, least likely to cause merge conflicts area) and we can see how it looks...
Comment From: yossigo
@ushachar Maybe it's even too simple, but you can give it a try!
Comment From: madolson
My motivation for splitting up server.[ch] is because I want to get to the point where we can be multi-threaded where threads own some of the slot space :), which will require splitting up the data into the various components.
I agree with the notion that a lot of this should be driven by building new functionality, but not being concerned about refactoring when it makes sense.