feat: dont store cmd(Get/Set/Hget/Hset) with too large key in cache #2849
Conversation
WalkthroughThe changes introduce a new configuration parameter to limit the maximum key size in the Redis cache, significantly enhancing cache management. Various command classes implement validations to prevent oversized keys from being stored, ensuring efficient memory usage. Integration tests have been added to verify correct behavior when encountering large keys. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant PikaClientConn
participant Cache
participant PikaConf
Client->>PikaClientConn: Send Set Command with Key
PikaClientConn->>PikaConf: Get max_key_size_in_cache
PikaClientConn->>PikaClientConn: Check if Key Size > max_key_size
alt Key Size Valid
PikaClientConn->>Cache: Store Key in Cache
else Key Size Too Large
PikaClientConn-->>Client: Reject Command
end
Assessment against linked issues
Possibly related PRs
Suggested labels
Poem
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (1)
Additional comments not posted (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
github-actions
bot
added
☢️ Bug
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
include/pika_conf.h (1)
415-415: Ensure thread safety inSetCacheMaxKeySize.The
SetCacheMaxKeySizemethod sets the value ofmax_key_size_in_cache_. Unlike other setter methods in the same file, it does not use a locking mechanism to ensure thread safety. Consider adding a lock guard to maintain consistency and thread safety.
include/pika_conf.h: Addstd::lock_guard l(rwlock_);toSetCacheMaxKeySize.Analysis chain
Ensure thread safety in
SetCacheMaxKeySize.The
SetCacheMaxKeySizemethod sets the value ofmax_key_size_in_cache_. Ensure that this method is consistent with other setter methods in terms of locking shared resources.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify consistency of locking mechanisms in setter methods. # Test: Search for setter methods and check for locking mechanisms. rg --type cpp -A 5 $'void Set'Length of output: 61456
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (11)
- conf/pika.conf (1 hunks)
- include/pika_command.h (1 hunks)
- include/pika_conf.h (3 hunks)
- include/pika_hash.h (1 hunks)
- include/pika_kv.h (2 hunks)
- src/cache/include/config.h (1 hunks)
- src/pika_client_conn.cc (1 hunks)
- src/pika_conf.cc (1 hunks)
- src/pika_hash.cc (2 hunks)
- src/pika_kv.cc (2 hunks)
- tests/integration/set_test.go (2 hunks)
Files skipped from review due to trivial changes (1)
- src/cache/include/config.h
Additional comments not posted (15)
include/pika_hash.h (1)
57-57: LGTM! The method implementation is correct.The method
IsTooLargeKeycorrectly checks if the key size exceeds the given maximum size.tests/integration/set_test.go (2)
408-427: LGTM! The test case correctly verifies that large keys are not stored in the cache.The test case creates a large key, sets a value, retrieves it, and checks the cache info to ensure the key is not cached.
431-442: LGTM! The helper function implementation is correct.The function
extractValuecorrectly parses the data and retrieves the value associated with the specified key.src/pika_client_conn.cc (1)
355-360: LGTM! The new check correctly handles commands with large keys.The check uses the
IsTooLargeKeymethod to determine if the key size exceeds the predefined limit. If the key is too large, the response number is decremented, and the method returnsfalse.include/pika_command.h (1)
532-532: LGTM! The pure virtual function enforces derived classes to implement key size validation.This change ensures that each command can have its own logic for determining if a key is too large.
src/pika_hash.cc (2)
78-81: LGTM! The key size validation optimizes the cache update process.The comment clarifies the purpose of the validation, and the check ensures that large keys are not stored in the cache.
130-132: LGTM! The key size validation enhances the robustness of the caching mechanism.The check ensures that large keys are not stored in the cache, improving performance and resource management.
include/pika_kv.h (2)
32-32: LGTM! The key size validation is correctly implemented.The method uses a static cast to convert
max_sztouint32_tfor comparison, ensuring correct functionality.
68-68: LGTM! The key size validation is correctly implemented.The method uses a static cast to convert
max_sztouint32_tfor comparison, ensuring correct functionality.conf/pika.conf (1)
571-574: New configuration parameter added:max-key-size-in-cache.This parameter specifies the maximum size of keys that can be stored in the Redis cache. The comments provide clear guidance on its usage, including the default value and the behavior when set to -1.
src/pika_conf.cc (1)
639-645: New configuration parameter handling:max-key-size-in-cache.The code correctly initializes the
max_key_size_in_cachevariable with the default value and updates it based on the configuration file. If the value is less than or equal to zero, it resets to the default value. This ensures that the parameter remains valid.include/pika_conf.h (2)
886-886: Ensure thread safety inmax_key_size_in_cache.The
max_key_size_in_cachemethod retrieves the value ofmax_key_size_in_cache_. Ensure that this method is consistent with other getter methods in terms of locking shared resources.
1036-1036: Ensure correct initialization ofmax_key_size_in_cache_.The
max_key_size_in_cache_member variable is added as anstd::atomic_intand initialized to 512. Ensure that this initialization is consistent with other member variables.Verification successful
Initialization of
max_key_size_in_cache_is consistent.The
max_key_size_in_cache_member variable is correctly initialized in a manner consistent with otherstd::atomic_intmember variables in the same file.
include/pika_conf.h:
std::atomic_int cache_num_ = 5;std::atomic_int cache_mode_ = 1;std::atomic_int cache_string_ = 1;std::atomic_int cache_set_ = 1;std::atomic_int cache_zset_ = 1;std::atomic_int cache_hash_ = 1;std::atomic_int cache_list_ = 1;std::atomic_int cache_bit_ = 1;std::atomic_int zset_cache_start_direction_ = 0;std::atomic_int zset_cache_field_num_per_key_ = 512;std::atomic_int cache_maxmemory_policy_ = 1;std::atomic_int cache_maxmemory_samples_ = 5;std::atomic_int cache_lfu_decay_time_ = 1;Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify consistency of initialization of member variables. # Test: Search for member variables and check for initialization. rg --type cpp -A 5 $'std::atomic_int'Length of output: 1793
src/pika_kv.cc (2)
109-111: LGTM! But verify the function and configuration usage.The added condition to check if the key size exceeds a configured maximum value before updating the cache is correct and aligns with the PR objectives.
However, ensure that the function
IsTooLargeKeyand the configurationg_pika_conf->max_key_size_in_cacheare correctly implemented and used.Verification successful
Verified: The function
IsTooLargeKeyand the configurationg_pika_conf->max_key_size_in_cacheare correctly implemented and used.
- The function
IsTooLargeKeychecks if the key size exceeds the configured maximum value.- The configuration parameter
g_pika_conf->max_key_size_in_cacheis properly defined and used to store the maximum key size.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation and usage of `IsTooLargeKey` and `g_pika_conf->max_key_size_in_cache`. # Test: Search for the function definition and configuration usage. Expect: Correct implementation and usage. rg --type cpp 'IsTooLargeKey' rg --type cpp 'max_key_size_in_cache'Length of output: 2086
188-190: LGTM! But verify the function and configuration usage.The added condition to check if the key size exceeds a configured maximum value before updating the cache is correct and aligns with the PR objectives.
However, ensure that the function
IsTooLargeKeyand the configurationg_pika_conf->max_key_size_in_cacheare correctly implemented and used.Verification successful
LGTM! The function and configuration usage are correctly implemented.
The added condition to check if the key size exceeds a configured maximum value before updating the cache is correct and aligns with the PR objectives. The function
IsTooLargeKeyand the configurationg_pika_conf->max_key_size_in_cacheare correctly implemented and used.
IsTooLargeKeycorrectly compares the key size with the maximum allowed size.max_key_size_in_cacheis properly defined and used in the configuration files.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation and usage of `IsTooLargeKey` and `g_pika_conf->max_key_size_in_cache`. # Test: Search for the function definition and configuration usage. Expect: Correct implementation and usage. rg --type cpp 'IsTooLargeKey' rg --type cpp 'max_key_size_in_cache'Length of output: 2086
Script:
#!/bin/bash # Extract the implementation of IsTooLargeKey from include/pika_kv.h rg --type cpp --context 5 'bool IsTooLargeKey' include/pika_kv.h # Extract the relevant code for max_key_size_in_cache from src/pika_conf.cc and include/pika_conf.h rg --type cpp --context 5 'max_key_size_in_cache' src/pika_conf.cc include/pika_conf.hLength of output: 4318
QlQlqiqi
force-pushed
the
big-key-not-in-cache
branch
from
c1ebe48 to
427c9ef
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (11)
- conf/pika.conf (1 hunks)
- include/pika_command.h (1 hunks)
- include/pika_conf.h (3 hunks)
- include/pika_hash.h (1 hunks)
- include/pika_kv.h (2 hunks)
- src/cache/include/config.h (1 hunks)
- src/pika_client_conn.cc (1 hunks)
- src/pika_conf.cc (1 hunks)
- src/pika_hash.cc (2 hunks)
- src/pika_kv.cc (2 hunks)
- tests/integration/set_test.go (2 hunks)
Files skipped from review as they are similar to previous changes (10)
- conf/pika.conf
- include/pika_command.h
- include/pika_conf.h
- include/pika_hash.h
- include/pika_kv.h
- src/cache/include/config.h
- src/pika_client_conn.cc
- src/pika_hash.cc
- src/pika_kv.cc
- tests/integration/set_test.go
Additional comments not posted (1)
src/pika_conf.cc (1)
639-645: LGTM! But verify the usage of the new configuration.The code changes are approved.
However, ensure that the new configuration
max-key-size-in-cacheis properly used throughout the codebase.
QlQlqiqi
changed the title
QlQlqiqi
force-pushed
the
big-key-not-in-cache
branch
from
427c9ef to
ba5903b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (12)
- conf/pika.conf (1 hunks)
- include/pika_command.h (1 hunks)
- include/pika_conf.h (3 hunks)
- include/pika_hash.h (1 hunks)
- include/pika_kv.h (2 hunks)
- src/cache/include/config.h (1 hunks)
- src/pika_client_conn.cc (1 hunks)
- src/pika_conf.cc (1 hunks)
- src/pika_hash.cc (2 hunks)
- src/pika_kv.cc (2 hunks)
- tests/integration/hash_test.go (2 hunks)
- tests/integration/set_test.go (2 hunks)
Files skipped from review as they are similar to previous changes (9)
- conf/pika.conf
- include/pika_command.h
- include/pika_conf.h
- include/pika_kv.h
- src/cache/include/config.h
- src/pika_client_conn.cc
- src/pika_conf.cc
- src/pika_hash.cc
- src/pika_kv.cc
Additional comments not posted (4)
tests/integration/hash_test.go (1)
369-388: LGTM!The test case effectively verifies that large keys are not stored in the cache. The logic is sound, and the use of
extractValueimproves readability.include/pika_hash.h (1)
57-57: LGTM!The
IsTooLargeKeymethod correctly checks if the key size exceeds the specified limit using a static cast for comparison.tests/integration/set_test.go (2)
408-427: LGTM!The test case effectively verifies that large keys are not stored in the cache. The logic is sound, and the use of
extractValueimproves readability.
431-442: LGTM!The
extractValuefunction is well-implemented and improves the readability of the test cases by encapsulating the logic for parsing the cache info string.
QlQlqiqi
force-pushed
the
big-key-not-in-cache
branch
from
ba5903b to
71d44f1
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (9)
- include/pika_command.h (1 hunks)
- include/pika_conf.h (3 hunks)
- include/pika_hash.h (1 hunks)
- include/pika_kv.h (2 hunks)
- src/cache/include/config.h (1 hunks)
- src/pika_client_conn.cc (1 hunks)
- src/pika_conf.cc (1 hunks)
- src/pika_hash.cc (2 hunks)
- src/pika_kv.cc (2 hunks)
Files skipped from review as they are similar to previous changes (8)
- include/pika_command.h
- include/pika_conf.h
- include/pika_hash.h
- include/pika_kv.h
- src/cache/include/config.h
- src/pika_conf.cc
- src/pika_hash.cc
- src/pika_kv.cc
Additional comments not posted (1)
src/pika_client_conn.cc (1)
355-360: Ensure theIsTooLargeKeymethod is efficient.The added code correctly checks if the key size exceeds the maximum allowed size before caching. However, ensure that the
IsTooLargeKeymethod is efficient and does not introduce significant overhead.Verification successful
Ensure the
IsTooLargeKeymethod is efficient.The
IsTooLargeKeymethod implementations inpika_hash.handpika_kv.hare efficient as they perform a simple size comparison. The implementation inpika_command.hreturnsfalseby default, which is likely intended for base class behavior.
include/pika_hash.h:bool IsTooLargeKey(const int &max_sz) override { return key_.size() > static_cast<uint32_t>(max_sz); }include/pika_kv.h:bool IsTooLargeKey(const int& max_sz) override { return key_.size() > static_cast<uint32_t>(max_sz); }include/pika_kv.h:bool IsTooLargeKey(const int &max_sz) override { return key_.size() > static_cast<uint32_t>(max_sz); }include/pika_command.h:virtual bool IsTooLargeKey(const int &max_sz) { return false; }The logic is correct and should not introduce significant overhead.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of `IsTooLargeKey` method. # Test: Search for the implementation of `IsTooLargeKey`. Expect: Efficient implementation. rg --type cpp --pattern 'bool IsTooLargeKey'Length of output: 458
Script:
#!/bin/bash # Description: Verify the implementation of `IsTooLargeKey` method. # Test: Search for the implementation of `IsTooLargeKey`. Expect: Efficient implementation. rg 'bool IsTooLargeKey'Length of output: 485
|
@chejinge @Mixficsol 本周review |
@chejinge @Mixficsol review this week |
src/pika_client_conn.cc
Outdated
| @@ -352,6 +352,12 @@ bool PikaClientConn::ReadCmdInCache(const net::RedisCmdArgsType& argv, const std | |||
| } | |||
| // Initial | |||
| c_ptr->Initial(argv, current_db_); | |||
| // dont store cmd with too large key(only Get/HGet cmd can reach here) | |||
| // the cmd with large key should be non-exist in cache, except for pre-stored | |||
| if(c_ptr->IsTooLargeKey(g_pika_conf->max_key_size_in_cache())) { | |||
fix: #2557
Summary by CodeRabbit
New Features
Bug Fixes
Tests