feat:zpopmax && zpopmin to do cache #2922
Conversation
WalkthroughThe changes in this pull request primarily involve modifications to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Outside diff range and nitpick comments (9)
src/cache/include/cache.h (3)
150-150: LGTM! Consider adding a brief comment.The
ZPopMinmethod signature looks good and is consistent with other methods in the class. It efficiently uses references and pointers for input/output parameters.Consider adding a brief comment explaining the method's functionality, e.g.:
// Removes and returns up to 'count' members with the lowest scores from the sorted set stored at 'key'.
151-151: LGTM! Consider adding a brief comment.The
ZPopMaxmethod signature is consistent withZPopMinand other methods in the class, which is excellent for maintainability and usability.Consider adding a brief comment explaining the method's functionality, e.g.:
// Removes and returns up to 'count' members with the highest scores from the sorted set stored at 'key'.
150-151: Great addition to Zset functionality. Consider grouping related methods.The addition of
ZPopMinandZPopMaxmethods enhances the RedisCache class's capabilities for sorted set operations. These methods are valuable for implementing priority queues, leaderboards, and other use cases requiring ordered data manipulation.Consider grouping related Zset methods together for better code organization. For example, you might want to place
ZPopMinandZPopMaxnear other methods that modify the sorted set, such asZAddorZRem.include/pika_zset.h (1)
606-607: LGTM! Consider adding brief comments for the new methods.The addition of
DoThroughDB()andDoUpdateCache()methods is a good improvement, allowing for better database and cache interaction. The use of theoverridekeyword is correct and follows best practices.Consider adding brief inline comments to describe the purpose of these methods, which would improve the code's self-documentation. For example:
// Performs the command operation through the database void DoThroughDB() override; // Updates the cache after the command execution void DoUpdateCache() override;src/pika_zset.cc (2)
1514-1519: Consider adding error handling and validating count_.The implementation looks good overall, but there are a couple of points to consider:
The method doesn't handle the case where
s_is not ok and not "NotFound". This might lead to unexpected behavior if there's an error.The
count_variable is used without checking its value. If it's negative or zero, it might lead to unexpected behavior.Consider adding error handling and validating
count_:void ZPopmaxCmd::DoUpdateCache() { std::vector<storage::ScoreMember> score_members; - if (s_.ok() || s_.IsNotFound()) { + if (s_.ok()) { + if (count_ > 0) { db_->cache()->ZPopMax(key_, count_, &score_members, db_); + } + } else if (!s_.IsNotFound()) { + // Log error or handle it appropriately } }
Line range hint
1510-1546: Summary of changes to ZPopmaxCmd and ZPopminCmdThe additions to
ZPopmaxCmdandZPopminCmdclasses implementDoThroughDBandDoUpdateCachemethods, which are consistent with the pattern used in other commands in this file. However, there are a few areas that could be improved:
Error Handling: Both
DoUpdateCachemethods could benefit from better error handling, especially for cases wheres_is not ok and not "NotFound".Input Validation: The
count_variable is used without validation in bothDoUpdateCachemethods. It should be checked to ensure it's positive before use.Code Duplication: There's significant similarity between the
DoUpdateCachemethods ofZPopmaxCmdandZPopminCmd. Consider refactoring to reduce duplication, possibly by creating a base class with a template method.Consistency: Ensure that the error handling and input validation are consistent across all similar methods in the file.
Consider creating a base class for Zset commands that implements common functionality, which could then be inherited by specific command classes like
ZPopmaxCmdandZPopminCmd. This would promote code reuse and ensure consistency across similar operations.src/pika_cache.cc (3)
1468-1481: Implementation looks good, with a minor note.The
ZPopMinfunction is well-implemented with proper thread safety and error handling. It correctly checks for the key's existence in the cache before performing the operation.The
dbparameter is currently unused in the function body. If it's intentional (e.g., for consistency with other method signatures or future use), consider adding a comment explaining its purpose or use the[[maybe_unused]]attribute to suppress potential compiler warnings.
1483-1496: Implementation is correct and consistent with ZPopMin.The
ZPopMaxfunction is well-implemented, mirroring the structure ofZPopMin. It ensures thread safety, proper error handling, and correctly delegates the operation to the cache object.
- The
dbparameter is unused, similar toZPopMin. Consider adding a comment or using the[[maybe_unused]]attribute.- Given the similarity between
ZPopMinandZPopMax, you might consider refactoring the common logic into a private helper function to reduce code duplication and improve maintainability.
1468-1496: Summary: Successful implementation of ZPopMin and ZPopMax functionalityThe addition of
ZPopMinandZPopMaxfunctions to thePikaCacheclass successfully implements the PR objectives. These functions enhance the cache's capabilities for sorted sets, allowing efficient pop operations for minimum and maximum elements.Key points:
- Both functions are correctly implemented with proper thread safety and error handling.
- The implementations are consistent with each other and with the existing coding style in the
PikaCacheclass.- The changes integrate well with the existing codebase and extend the cache functionality as intended.
To further improve the code:
- Consider refactoring the common logic in
ZPopMinandZPopMaxinto a private helper function to reduce duplication.- Address the unused
dbparameter in both functions, either by adding a comment explaining its purpose or by using the[[maybe_unused]]attribute.These changes have a positive impact on the codebase by expanding the cache's functionality for sorted sets, which can potentially improve performance for certain operations.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
- include/pika_cache.h (1 hunks)
- include/pika_zset.h (2 hunks)
- src/cache/include/cache.h (1 hunks)
- src/cache/src/zset.cc (1 hunks)
- src/pika_cache.cc (1 hunks)
- src/pika_command.cc (1 hunks)
- src/pika_zset.cc (2 hunks)
🧰 Additional context used
🔇 Additional comments (14)
include/pika_zset.h (1)
628-629: LGTM! Consistent with ZPopmaxCmd changes.The addition of
DoThroughDB()andDoUpdateCache()methods inZPopminCmdis consistent with the changes made toZPopmaxCmd, which is good for maintaining symmetry between these related commands.As mentioned in the comment for
ZPopmaxCmd, consider adding brief inline comments to describe the purpose of these methods.src/pika_zset.cc (2)
1510-1512: Implementation looks good.The
DoThroughDBmethod correctly calls theDo()method, which is consistent with the pattern used in other commands in this file.
1537-1539: Implementation looks good.The
DoThroughDBmethod correctly calls theDo()method, which is consistent with the pattern used in other commands in this file.src/pika_command.cc (2)
603-603: Approved: Enhanced ZPopmaxCmd with cache interactionThe
ZPopmaxCmdconstructor has been updated to include two new flags:kCmdFlagsDoThroughDBandkCmdFlagsUpdateCache. These additions improve the command's interaction with the cache:
kCmdFlagsDoThroughDBensures that the command interacts directly with the database, maintaining data consistency.kCmdFlagsUpdateCacheallows the cache to be updated after the database operation, keeping the cache in sync.These changes align well with the PR objective of implementing cache functionality for the
zpopmaxcommand, potentially improving performance while maintaining data integrity.
607-607: Approved: Consistent enhancement of ZPopminCmd with cache interactionThe
ZPopminCmdconstructor has been updated in a manner consistent with theZPopmaxCmdchanges. The addition ofkCmdFlagsDoThroughDBandkCmdFlagsUpdateCacheflags ensures that:
- The command interacts directly with the database, maintaining data consistency.
- The cache is updated after the database operation, keeping it in sync.
This consistency between
zpopminandzpopmaxis commendable, as these complementary commands should have similar behavior with respect to cache interactions.include/pika_cache.h (9)
152-153: Addition of parameters toZIncrbyIfKeyExistmethod is appropriateThe updated method signature with the added parameters
ZIncrbyCmd* cmdandconst std::shared_ptr<DB>& dbis consistent with other methods and enhances functionality.
154-155: Update toZRangemethod signature is appropriate and consistentAdding
const std::shared_ptr<DB>& dbto theZRangemethod aligns with other method signatures and allows for enhanced database interaction.
163-164: Update toZRevrangemethod signature is appropriate and consistentAdding
const std::shared_ptr<DB>& dbto theZRevrangemethod ensures consistency and facilitates database transactions.
168-169: Update toZRevrangebylexmethod signature is appropriate and consistentIncluding
const std::shared_ptr<DB>& dbin theZRevrangebylexmethod aligns it with the other methods and supports database interactions.
170-170: Update toZRevrankmethod signature is appropriate and consistentAdding
const std::shared_ptr<DB>& dbto theZRevrankmethod ensures consistency across the class methods.
172-173: Update toZRangebylexmethod signature is appropriate and consistentIncluding
const std::shared_ptr<DB>& dbaligns theZRangebylexmethod with other methods requiring database references.
177-178: Addition ofZPopMinmethod is appropriate and aligns with class functionalityThe new method
ZPopMinis correctly added, enhancing the sorted set operations.
179-180: Addition ofZPopMaxmethod is appropriate and aligns with class functionalityThe new method
ZPopMaxcomplements the sorted set capabilities.
152-173: Verify all usages of updated methods have been updated accordinglyThe methods
ZIncrbyIfKeyExist,ZRange,ZRevrange,ZRevrangebylex,ZRank, andZRangebylexhave updated signatures with additional parameters. Please ensure that all calls to these methods in the codebase have been updated to match the new signatures.Run the following script to find all usages of these methods:
| void ZPopminCmd::DoUpdateCache() { | ||
| std::vector<storage::ScoreMember> score_members; | ||
| if (s_.ok() || s_.IsNotFound()) { | ||
| db_->cache()->ZPopMin(key_, count_, &score_members, db_); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding error handling, validating count_, and reusing code.
The implementation looks good overall, but there are several points to consider:
-
The method doesn't handle the case where
s_is not ok and not "NotFound". This might lead to unexpected behavior if there's an error. -
The
count_variable is used without checking its value. If it's negative or zero, it might lead to unexpected behavior. -
This method is very similar to
ZPopmaxCmd::DoUpdateCache, suggesting a potential for code reuse.
Consider the following improvements:
- Add error handling and validate
count_:
void ZPopminCmd::DoUpdateCache() {
std::vector<storage::ScoreMember> score_members;
- if (s_.ok() || s_.IsNotFound()) {
+ if (s_.ok()) {
+ if (count_ > 0) {
db_->cache()->ZPopMin(key_, count_, &score_members, db_);
+ }
+ } else if (!s_.IsNotFound()) {
+ // Log error or handle it appropriately
}
}- To reduce code duplication, consider creating a template method in a base class that both
ZPopmaxCmdandZPopminCmdcan inherit from. This would allow you to reuse the same code for bothZPopMaxandZPopMinoperations.
Committable suggestion was skipped due to low confidence.
| kCmdNameZPopmax, -2, kCmdFlagsWrite | kCmdFlagsZset | kCmdFlagsFast | kCmdFlagsDoThroughDB | kCmdFlagsUpdateCache); | ||
| cmd_table->insert(std::pair<std::string, std::unique_ptr<Cmd>>(kCmdNameZPopmax, std::move(zpopmaxptr))); | ||
| ////ZPopmin | ||
| std::unique_ptr<Cmd> zpopminptr = std::make_unique<ZPopminCmd>( | ||
| kCmdNameZPopmin, -2, kCmdFlagsWrite | kCmdFlagsZset | kCmdFlagsFast); | ||
| kCmdNameZPopmin, -2, kCmdFlagsWrite | kCmdFlagsZset | kCmdFlagsFast | kCmdFlagsDoThroughDB | kCmdFlagsUpdateCache); |
There was a problem hiding this comment.
💡 Codebase verification
Inconsistency Found in Zset Commands Flag Usage
While ZPopmaxCmd and ZPopminCmd have been updated with the kCmdFlagsDoThroughDB and kCmdFlagsUpdateCache flags, several other Zset commands are missing these flags. This inconsistency could lead to unexpected behavior or performance issues.
Affected Files:
include/pika_zset.h:ZAddCmdZCardCmdZIncrbyCmdZRangeCmdZRevrangeCmdZCountCmdZRemCmdZUnionstoreCmdZInterstoreCmdZRankCmdZRevrankCmdZScoreCmdZRangebylexCmdZLexcountCmdZRemrangebyrankCmdZRemrangebyscoreCmdZRemrangebylexCmdZScanCmdZRangebyscoreCmdZRevrangebyscoreCmd
Please update these commands to include the kCmdFlagsDoThroughDB and kCmdFlagsUpdateCache flags to ensure consistent behavior across all Zset operations.
🔗 Analysis chain
Summary: Effective implementation of cache functionality for zpopmax and zpopmin
The changes to both ZPopmaxCmd and ZPopminCmd constructors successfully implement cache interaction for these commands. By adding the kCmdFlagsDoThroughDB and kCmdFlagsUpdateCache flags, the modifications ensure that:
- Both commands will interact directly with the database, maintaining data consistency.
- The cache will be updated after database operations, keeping it in sync.
These changes are consistent across both commands and align well with the PR objective. The implementation should improve performance while maintaining data integrity for sorted set operations.
Consider adding unit tests to verify the new cache interaction behavior for both commands.
To ensure the changes are properly integrated, run the following verification script:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the new flags are used consistently across the codebase
echo "Checking usage of new flags:"
rg "kCmdFlagsDoThroughDB|kCmdFlagsUpdateCache" --type cpp
# Verify if there are any other Zset commands that might need similar updates
echo "Checking other Zset commands:"
rg "std::make_unique<Z\w+Cmd>" --type cpp
Length of output: 19379
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (14)
src/cache/src/zset.cc (1)
408-475: Overall implementation review forZPopMinandZPopMax.The implementation of
ZPopMinandZPopMaxachieves the basic functionality of popping minimum and maximum elements from sorted sets. However, there are several areas for improvement:
- Efficiency: Both methods retrieve all items from the sorted set, which can be inefficient for large sets. Consider using range-limited retrieval.
- Error handling: The return value of
RcZRemis not checked in both methods, which could lead to undetected errors.- Memory management: In
ZPopMin, memory allocation formembers_objcan be optimized.- Loop and indexing issues: In
ZPopMax, there's a potential infinite loop and incorrect indexing when populatingmembers_obj.Addressing these issues will improve the robustness and efficiency of the implementation.
Consider creating a helper function to handle the common logic between
ZPopMinandZPopMax, parameterizing the differences (e.g., range retrieval function, indexing). This would reduce code duplication and make maintenance easier.src/pika_zset.cc (2)
1514-1519: Implementation looks good, with a minor optimization possibilityThe
DoUpdateCachemethod correctly updates the cache when the status is ok or NotFound. It's good that it checks for both conditions.One minor suggestion:
If thescore_membersvector is not used after theZPopMaxcall, you could consider passing a nullptr instead to avoid unnecessary memory allocation:- std::vector<storage::ScoreMember> score_members; if (s_.ok() || s_.IsNotFound()) { - db_->cache()->ZPopMax(key_, count_, &score_members, db_); + db_->cache()->ZPopMax(key_, count_, nullptr, db_); }This change would be more efficient if the popped elements are not needed for any further operations in the cache update.
1541-1546: Implementation looks good, with a minor optimization possibilityThe
DoUpdateCachemethod correctly updates the cache when the status is ok or NotFound, which is consistent with theZPopmaxCmdimplementation.As with the
ZPopmaxCmd, if thescore_membersvector is not used after theZPopMincall, consider passing a nullptr instead to avoid unnecessary memory allocation:- std::vector<storage::ScoreMember> score_members; if (s_.ok() || s_.IsNotFound()) { - db_->cache()->ZPopMin(key_, count_, &score_members, db_); + db_->cache()->ZPopMin(key_, count_, nullptr, db_); }This change would be more efficient if the popped elements are not needed for any further operations in the cache update.
src/pika_cache.cc (2)
1468-1481: Implementation looks good, but consider improving error handling and cache warming.The
ZPopMinfunction is correctly implemented with proper thread safety and key existence check. However, there are a couple of areas that could be improved:
Error handling: The function doesn't handle the case where the key exists but the
ZPopMinoperation on the cache object fails. Consider adding error handling for this scenario.Cache warming: When the key is not found in the cache, this could be an opportunity to load it from the database and warm up the cache. Consider implementing a cache warming strategy in this case.
Here's a suggested improvement:
Status PikaCache::ZPopMin(std::string &key, int64_t count, std::vector<storage::ScoreMember> *score_members, const std::shared_ptr<DB> &db) { int cache_index = CacheIndex(key); std::lock_guard lm(*cache_mutexs_[cache_index]); auto cache_obj = caches_[cache_index]; Status s; if (cache_obj->Exists(key)) { - return cache_obj->ZPopMin(key, count, score_members); + s = cache_obj->ZPopMin(key, count, score_members); + if (!s.ok()) { + // Handle the error case + // For example, log the error or take appropriate action + } + return s; } else { + // Consider implementing cache warming here + // For example: + // s = LoadZSetFromDB(key, db); + // if (s.ok()) { + // return cache_obj->ZPopMin(key, count, score_members); + // } return Status::NotFound("key not in cache"); } }
1483-1496: Implementation is consistent with ZPopMin, consider applying the same improvements.The
ZPopMaxfunction is implemented consistently withZPopMin, which is good for code maintainability. However, it shares the same areas for improvement:
- Error handling: Add handling for the case where the key exists but the
ZPopMaxoperation fails.- Cache warming: Consider implementing a cache warming strategy when the key is not found in the cache.
Apply the same improvements suggested for
ZPopMin:Status PikaCache::ZPopMax(std::string &key, int64_t count, std::vector<storage::ScoreMember> *score_members, const std::shared_ptr<DB> &db) { int cache_index = CacheIndex(key); std::lock_guard lm(*cache_mutexs_[cache_index]); auto cache_obj = caches_[cache_index]; Status s; if (cache_obj->Exists(key)) { - return cache_obj->ZPopMax(key, count, score_members); + s = cache_obj->ZPopMax(key, count, score_members); + if (!s.ok()) { + // Handle the error case + // For example, log the error or take appropriate action + } + return s; } else { + // Consider implementing cache warming here + // For example: + // s = LoadZSetFromDB(key, db); + // if (s.ok()) { + // return cache_obj->ZPopMax(key, count, score_members); + // } return Status::NotFound("key not in cache"); } }Additionally, consider extracting the common logic of
ZPopMinandZPopMaxinto a private helper function to reduce code duplication.tests/integration/zset_test.go (1)
1095-1125: Excellent addition of the "should Zpopmin test" case.This test case effectively covers both ZPopMax and ZPopMin operations, aligning well with the PR objectives. It follows the existing test patterns and provides good coverage of the functionality.
A minor suggestion for improvement:
Consider adding an assertion to check the length of the remaining set after the ZPopMax and ZPopMin operations. This would provide an additional verification that the set size has decreased as expected. You can add this assertion just before the final ZRange check:
// Add this before the ZRange check setSize, err := client.ZCard(ctx, "zpopzset1").Result() Expect(err).NotTo(HaveOccurred()) Expect(setSize).To(Equal(int64(1)))This addition would further strengthen the test by ensuring that the set size is correct after the pop operations.
include/pika_cache.h (8)
152-153: Consider passing 'key' and 'member' asconstreferencesTo improve efficiency and prevent unintended modifications, consider changing the parameters
keyandmembertoconst std::string&.Apply this diff:
-rocksdb::Status ZIncrbyIfKeyExist(std::string& key, std::string& member, double increment, ZIncrbyCmd* cmd, +rocksdb::Status ZIncrbyIfKeyExist(const std::string& key, const std::string& member, double increment, ZIncrbyCmd* cmd, const std::shared_ptr<DB>& db);
154-155: Consider passing 'key' as aconstreferenceFor consistency and efficiency, consider changing the parameter
keytoconst std::string&.Apply this diff:
-rocksdb::Status ZRange(std::string& key, int64_t start, int64_t stop, +rocksdb::Status ZRange(const std::string& key, int64_t start, int64_t stop, std::vector<storage::ScoreMember>* score_members, const std::shared_ptr<DB>& db);
163-164: Consider passing 'key' as aconstreferenceTo improve const correctness and prevent unintended side effects, consider changing the parameter
keytoconst std::string&.Apply this diff:
-rocksdb::Status ZRevrange(std::string& key, int64_t start, int64_t stop, +rocksdb::Status ZRevrange(const std::string& key, int64_t start, int64_t stop, std::vector<storage::ScoreMember>* score_members, const std::shared_ptr<DB>& db);
168-169: Consider passing string parameters asconstreferencesFor improved efficiency and to prevent unintended modifications, consider changing the parameters
key,min, andmaxtoconst std::string&.Apply this diff:
-rocksdb::Status ZRevrangebylex(std::string& key, std::string& min, std::string& max, +rocksdb::Status ZRevrangebylex(const std::string& key, const std::string& min, const std::string& max, std::vector<std::string>* members, const std::shared_ptr<DB>& db);
170-170: Consider passing 'key' and 'member' asconstreferencesTo enhance const correctness and ensure the parameters are not modified, consider changing
keyandmembertoconst std::string&.Apply this diff:
-rocksdb::Status ZRevrank(std::string& key, std::string& member, int64_t* rank, const std::shared_ptr<DB>& db); +rocksdb::Status ZRevrank(const std::string& key, const std::string& member, int64_t* rank, const std::shared_ptr<DB>& db);
172-173: Consider passing string parameters asconstreferencesFor consistency and to prevent unintended changes, consider changing the parameters
key,min, andmaxtoconst std::string&.Apply this diff:
-rocksdb::Status ZRangebylex(std::string& key, std::string& min, std::string& max, std::vector<std::string>* members, +rocksdb::Status ZRangebylex(const std::string& key, const std::string& min, const std::string& max, std::vector<std::string>* members, const std::shared_ptr<DB>& db);
177-178: Consider passing 'key' as aconstreferenceTo improve efficiency and maintain const correctness, consider changing the parameter
keytoconst std::string&.Apply this diff:
-rocksdb::Status ZPopMin(std::string& key, int64_t count, std::vector<storage::ScoreMember>* score_members, +rocksdb::Status ZPopMin(const std::string& key, int64_t count, std::vector<storage::ScoreMember>* score_members, const std::shared_ptr<DB>& db);
179-180: Consider passing 'key' as aconstreferenceFor consistency and to prevent unintended modifications, consider changing the parameter
keytoconst std::string&.Apply this diff:
-rocksdb::Status ZPopMax(std::string& key, int64_t count, std::vector<storage::ScoreMember>* score_members, +rocksdb::Status ZPopMax(const std::string& key, int64_t count, std::vector<storage::ScoreMember>* score_members, const std::shared_ptr<DB>& db);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- include/pika_cache.h (1 hunks)
- src/cache/src/zset.cc (1 hunks)
- src/pika_cache.cc (1 hunks)
- src/pika_zset.cc (2 hunks)
- tests/integration/zset_test.go (1 hunks)
🧰 Additional context used
🔇 Additional comments (3)
src/pika_zset.cc (2)
1510-1512: Implementation looks goodThe
DoThroughDBmethod correctly calls theDo()method, which is consistent with the pattern used in other similar commands in this file.
1537-1539: Implementation looks goodThe
DoThroughDBmethod correctly calls theDo()method, which is consistent with the implementation inZPopmaxCmdand other similar commands in this file.tests/integration/zset_test.go (1)
Line range hint
1-1126: Overall, the changes in this file are well-implemented and valuable.The addition of the "should Zpopmin test" case enhances the test coverage for the zpopmin and zpopmax commands, which directly addresses the PR objectives. The test is well-structured and follows the existing patterns in the file.
The changes in this file are approved, with a minor suggestion for improvement in the new test case.
| robj **members_obj = (robj **)zcallocate(sizeof(robj *) * items_size); | ||
| for (unsigned long i = 0; i < items_size; ++i) { | ||
| members_obj[i] = createObject(OBJ_STRING, sdsnewlen(items[i].member, sdslen(items[i].member))); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize memory allocation for members_obj.
members_obj is allocated with items_size elements, but only to_return elements are used.
Allocate only the necessary memory:
-robj **members_obj = (robj **)zcallocate(sizeof(robj *) * items_size);
-for (unsigned long i = 0; i < items_size; ++i) {
+robj **members_obj = (robj **)zcallocate(sizeof(robj *) * to_return);
+for (unsigned long i = 0; i < to_return; ++i) {
members_obj[i] = createObject(OBJ_STRING, sdsnewlen(items[i].member, sdslen(items[i].member)));
}This change reduces memory usage and improves efficiency.
Committable suggestion was skipped due to low confidence.
fix issue #2892 (执行zpopmin后还能查询到被删除数据)
1.zpopmin与zpopmax作为写操作不会更新到cache值，导致对cache进行读操作的指令能读到被zpopmin删除的值，因此为zpopmin与zpopmax添加了更新cache的功能。
2.新增对应的测试case
Summary by CodeRabbit
Summary by CodeRabbit
New Features
ZPopMinandZPopMaxmethods to remove and retrieve elements from sorted sets based on scores.ZPopmaxCmdandZPopminCmdwith new methods for database interaction and cache updates.Bug Fixes
Refactor