Refactor debug configuration options for clarity and consistency #690
Conversation
Also make `enable-debug-assert` an immutable config Signed-off-by: Ping Xie <[email protected]>
Signed-off-by: Ping Xie <[email protected]>
to avoid restarting Valkey server during the test. The `DEBUG RESTART` procedure was not always reliable, as sometimes the server would fail to restart, leading to flaky tests. Signed-off-by: Ping Xie <[email protected]>
…ca versions. Added logic to iterate through the list of replicas and check for any replicas running a version older than 8.0.0. Older replicas do not support the CLUSTER SETSLOT command on replicas. If such a replica is found, the replication is skipped and falls back to the old (non-replicated) behavior. Signed-off-by: Ping Xie <[email protected]>
Signed-off-by: Ping Xie <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## unstable #690 +/- ##
============================================
- Coverage 70.21% 70.19% -0.02%
============================================
Files 110 110
Lines 60100 60104 +4
============================================
- Hits 42202 42193 -9
- Misses 17898 17911 +13
|
…ug-assert Signed-off-by: Ping Xie <[email protected]>
PingXie
changed the title
Signed-off-by: Ping Xie <[email protected]>
| char *logfile; /* Path of log file */ | ||
| int syslog_enabled; /* Is syslog enabled? */ | ||
| char *syslog_ident; /* Syslog ident */ | ||
| int syslog_facility; /* Syslog facility */ | ||
| int crashlog_enabled; /* Enable signal handler for crashlog. | ||
| * disable for clean core dumps. */ | ||
| int memcheck_enabled; /* Enable memory check on crash. */ |
There was a problem hiding this comment.
Unsure why these got modified.
There was a problem hiding this comment.
I know why :). It is clang-format aligning trailing comments. let me try out the LEAVE option and if it works I can add to this pr too
There was a problem hiding this comment.
Did this end up working for you? I was trying the LEAVE option and getting some very weird results that I couldn't really understand. It felt like it was just randomly indenting. What was worse is the results were different for clang-19 and clang-18.
There was a problem hiding this comment.
LEAVE didn't work for me. I noticed differences between clang-19 and clang-18 too but didn't dig further.
There was a problem hiding this comment.
Sigh. The constantly shifting comment indentation is annoying but probably something to figure out after the release candidates.
| char *debug_req_res_logfile; /* Path of log file for logging all requests and their replies. | ||
| * If NULL, no logging will be performed */ | ||
| unsigned int debug_client_default_resp; | ||
| #endif | ||
| int debug_assert_enabled; /* Is debug assert enabled? */ | ||
| int debug_rdb_key_save_delay; /* Delay in microseconds between keys while | ||
| * writing aof or rdb. (for testings). negative | ||
| * value means fractions of microseconds (on average). */ | ||
| int debug_key_load_delay; /* Delay in microseconds between keys while | ||
| * loading aof or rdb. (for testings). negative | ||
| * value means fractions of microseconds (on average). */ | ||
| int debug_aof_disable_auto_gc; /* If disable automatically deleting HISTORY type AOFs? | ||
| * default no. (for testings). */ | ||
| int debug_use_exit_on_panic; /* Use exit() on panic and assert rather than | ||
| * abort(). useful for Valgrind. */ | ||
| int debug_cluster_drop_packet_filter; /* Debug flag to enable dropping specific packet types. */ | ||
| int debug_watchdog_period; /* Software watchdog period in ms. 0 = off */ | ||
| mstime_t debug_cluster_ping_interval; /* Debug config for setting how often cluster nodes send ping messages. */ | ||
| off_t debug_loading_process_events_interval_bytes; | ||
| struct { | ||
| uint32_t debug_cluster_close_link_on_packet_drop : 1; /* Debug flag: close link on packet drop */ | ||
| uint32_t debug_pause_cron : 1; /* Debug flag: pause cron tasks */ | ||
| }; |
There was a problem hiding this comment.
Should we prefix each field with debug_ or should we place them within a struct debug_fields?
There was a problem hiding this comment.
should we place them within a struct debug_fields?
good question. I actually am in slight favor of a flat set of debug_ fields so I get to type less. thoughts?
There was a problem hiding this comment.
I use an IDE 😅, I'm fine with either.
There was a problem hiding this comment.
So, hot take, I think we should leave the configs where they are in the struct and keep the debug_ prefix. My general understand of why we group similar fields together is they are accessed together, and improves cache line hits. It's probably too small to measure, but it seems like the right type of mentality we should be following. I'm also not sure what is the benefit of grouping debug configs together.
There was a problem hiding this comment.
cache line aside, it is also cleaner to group debug_ settings and since we already change the names, where they belong isn't that an issue IMO. Let's keep them?
There was a problem hiding this comment.
Let's keep them?
I'm fine keeping the debug_ prefix. The only thing I don't like now is realizing we inconsistently use the term debug. (Sometimes it means debug command, sometimes it's for debugging purposes, sometimes it's for testing). I don't feel strongly though.
I would still rather put debug flags logically near the system they are a part of, instead of grouping all debug configs together. The only exception I see for grouping stuff together is if we were to group a large number of single bit values together to have a more pact cache line.
There was a problem hiding this comment.
We can move all the boolean-like debug vars together and make them 1-bit. That saves memory and improves cache locality since we have more vars in fewer cache lines. We can't if they're configs?
When do we start using bool for all the bool configs?
I'm not sure about rename them since it affects many places where they're accessed. It seems unnecessary (unless the names are really bad).
No description provided.