decouple lru/lfu from server.h#2928
Conversation
1f82e64 to
d38ef8d
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## unstable #2928 +/- ##
============================================
+ Coverage 72.45% 72.47% +0.02%
============================================
Files 129 129
Lines 70524 70537 +13
============================================
+ Hits 51095 51123 +28
+ Misses 19429 19414 -15
🚀 New features to boost your workflow:
|
rjd15372
left a comment
There was a problem hiding this comment.
Apart from my two comments, everything else looks good.
sarthakaggarwal97
left a comment
There was a problem hiding this comment.
Minor comments from my end. LGTM otherwise. Thank you @JimB123!
| createIntConfig("active-defrag-cycle-us", NULL, MODIFIABLE_CONFIG, 0, 100000, server.active_defrag_cycle_us, 500, INTEGER_CONFIG, NULL, updateDefragConfiguration), | ||
| createIntConfig("lfu-log-factor", NULL, MODIFIABLE_CONFIG, 0, INT_MAX, server.lfu_log_factor, 10, INTEGER_CONFIG, NULL, NULL), | ||
| createIntConfig("lfu-decay-time", NULL, MODIFIABLE_CONFIG, 0, INT_MAX, server.lfu_decay_time, 1, INTEGER_CONFIG, NULL, NULL), | ||
| createIntConfig("lfu-log-factor", NULL, MODIFIABLE_CONFIG, 0, INT_MAX, config_lfu_log_factor, 10, INTEGER_CONFIG, NULL, NULL), |
There was a problem hiding this comment.
I am not sure if it's an issue, but it looks like we are deviating from the current code structure where we do not have these configs as a part of server. Also config_ prefix is probably not required.
There was a problem hiding this comment.
I'm assuming that existing code simply followed the pattern that all config was tossed into the global server. object. I don't think there's any reason that this needs to be the case. Requiring all configs to be added to this global structure impacts the ability to create modularity.
If we think this is important, the correct way to do this would be to remove all configuration values from server. and create a simple structure (in it's own header file) which just contains configuration. That would avoid the need to pull in (the enormous) server.h for a simple config.
The config_ prefix is just for readability within this module, that the global is intended as a configuration value (rather than modified through some other means). However, based on this feedback, I changed the prefix to lfu_, matching the prefix on the LFU functions.
d38ef8d to
a5febad
Compare
Signed-off-by: Jim Brunner <brunnerj@amazon.com>
a5febad to
bf8c568
Compare
Addresses: valkey-io#2908 This decouples the low-level LRU/LFU code from the high-level `server.h` file. * LRU/LFU specific config values were removed from `server.h` and relocated to `lrulfu.h` * A new LRULFU API was created to update the time (and policy) rather than accessing globals from `server.` This doesn't address the root of the original test failure in that `server.h` may compile with different offsets (due to `off_t`) based on the include order. See: valkey-io#2908 (comment) Signed-off-by: Jim Brunner <brunnerj@amazon.com>
Addresses: valkey-io#2908 This decouples the low-level LRU/LFU code from the high-level `server.h` file. * LRU/LFU specific config values were removed from `server.h` and relocated to `lrulfu.h` * A new LRULFU API was created to update the time (and policy) rather than accessing globals from `server.` This doesn't address the root of the original test failure in that `server.h` may compile with different offsets (due to `off_t`) based on the include order. See: valkey-io#2908 (comment) Signed-off-by: Jim Brunner <brunnerj@amazon.com> Signed-off-by: Harkrishn Patro <bunty.hari@gmail.com>
Addresses: #2908
This decouples the low-level LRU/LFU code from the high-level
server.hfile.server.hand relocated tolrulfu.hserver.This doesn't address the root of the original test failure in that
server.hmay compile with different offsets (due tooff_t) based on the include order. See: #2908 (comment)