-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add new hash expire cmd to pika #2883
base: unstable
Are you sure you want to change the base?
feat: add new hash expire cmd to pika #2883
Conversation
1. pkhget pkhset 2. pkhexpire pkhexpireat 3. pkhexpiretime pkhpersist 4. pkhttl
WalkthroughThe changes introduce a new "PKHash" data type along with associated commands in the access control and storage systems. Key modifications include the addition of enumerators, command constants, and methods for managing PKHashes, which enhance the functionality of both the command processing framework and the storage backend. Additionally, formatting improvements and organizational adjustments are made across various files for clarity and readability. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CommandHandler
participant Storage
User->>CommandHandler: PKHSet(key, field, value)
CommandHandler->>Storage: Store key, field, value
Storage-->>CommandHandler: Confirmation
CommandHandler-->>User: Success
sequenceDiagram
participant User
participant CommandHandler
participant Storage
User->>CommandHandler: PKHGet(key, field)
CommandHandler->>Storage: Retrieve key, field
Storage-->>CommandHandler: Value, metadata
CommandHandler-->>User: Value or error
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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
Outside diff range, codebase verification and nitpick comments (14)
src/storage/src/redis_pkhashes.cc (7)
23-63
: Consider adding logging for better debugging and monitoring.The function is correctly implemented with proper error handling and validation checks. Adding logging can help in better debugging and monitoring.
Consider adding logging statements at key points in the function.
66-142
: Consider adding logging for better debugging and monitoring.The function is correctly implemented with proper error handling and validation checks. Adding logging can help in better debugging and monitoring.
Consider adding logging statements at key points in the function.
144-205
: Consider adding logging for better debugging and monitoring.The function is correctly implemented with proper error handling and validation checks. Adding logging can help in better debugging and monitoring.
Consider adding logging statements at key points in the function.
207-274
: Consider adding logging for better debugging and monitoring.The function is correctly implemented with proper error handling and validation checks. Adding logging can help in better debugging and monitoring.
Consider adding logging statements at key points in the function.
276-333
: Consider adding logging for better debugging and monitoring.The function is correctly implemented with proper error handling and validation checks. Adding logging can help in better debugging and monitoring.
Consider adding logging statements at key points in the function.
335-394
: Consider adding logging for better debugging and monitoring.The function is correctly implemented with proper error handling and validation checks. Adding logging can help in better debugging and monitoring.
Consider adding logging statements at key points in the function.
397-452
: Consider adding logging for better debugging and monitoring.The function is correctly implemented with proper error handling and validation checks. Adding logging can help in better debugging and monitoring.
Consider adding logging statements at key points in the function.
src/storage/src/storage.cc (6)
480-484
: Review Error Handling inGetDBInstance
The
PKHExpire
function includes error handling for invalid TTL values, which is appropriate. However, theGetDBInstance
function lacks explicit error handling, which might be a concern if the slot index retrieval or instance access could fail. Consider adding error handling toGetDBInstance
to ensure robustness.
GetDBInstance
: No explicit error handling observed.PKHExpire
: Handles invalid TTL values correctly.Analysis chain
LGTM! Verify error handling.
The function is correctly implemented. Ensure that
GetDBInstance
andPKHExpire
handle errors correctly.The code changes are approved.
Run the following script to verify error handling:
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify error handling in `GetDBInstance` and `PKHExpire`. # Test: Search for the function implementations. Expect: Proper error handling. rg --type cpp -A 5 $'std::unique_ptr<Redis>& Storage::GetDBInstance' rg --type cpp -A 5 $'Status Redis::PKHExpire'Length of output: 2096
486-490
: Potential Issue: Lack of Error Handling inGetDBInstance
The
GetDBInstance
function does not have explicit error handling, which could lead to issues if an invalid instance is returned. Consider adding error handling to ensure robustness. ThePKHExpireat
function inRedis
correctly handles invalid timestamps.
- Review the
GetDBInstance
function for potential error handling improvements.Analysis chain
LGTM! Verify error handling.
The function is correctly implemented. Ensure that
GetDBInstance
andPKHExpireat
handle errors correctly.The code changes are approved.
Run the following script to verify error handling:
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify error handling in `GetDBInstance` and `PKHExpireat`. # Test: Search for the function implementations. Expect: Proper error handling. rg --type cpp -A 5 $'std::unique_ptr<Redis>& Storage::GetDBInstance' rg --type cpp -A 5 $'Status Redis::PKHExpireat'Length of output: 1159
Script:
#!/bin/bash # Description: Check for error handling related to `GetDBInstance`. # Search for any error handling or checks related to `GetDBInstance`. rg --type cpp 'GetDBInstance' -A 3 | grep -i 'if\|try\|catch\|error\|exception'Length of output: 1873
504-508
: Improve Error Handling inGetDBInstance
The
GetDBInstance
function lacks explicit error handling, which could lead to issues if an invalid index is accessed. Consider adding error checks to handle such cases. ThePKHTTL
function inRedis
has partial error handling for specific conditions, which is a positive aspect.
- File:
src/storage/src/storage.cc
- Function:
GetDBInstance
Analysis chain
LGTM! Verify error handling.
The function is correctly implemented. Ensure that
GetDBInstance
andPKHTTL
handle errors correctly.The code changes are approved.
Run the following script to verify error handling:
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify error handling in `GetDBInstance` and `PKHTTL`. # Test: Search for the function implementations. Expect: Proper error handling. rg --type cpp -A 5 $'std::unique_ptr<Redis>& Storage::GetDBInstance' rg --type cpp -A 5 $'Status Redis::PKHTTL'Length of output: 1084
Script:
#!/bin/bash # Description: Gather more information on error handling in `GetDBInstance` and `PKHTTL`. # Check for error handling in `GetDBInstance`. rg --type cpp -A 10 'std::unique_ptr<Redis>& Storage::GetDBInstance' # Check for error handling in `PKHTTL`. rg --type cpp -A 20 'Status Redis::PKHTTL'Length of output: 2518
510-513
: Ensure Robust Error Handling inGetDBInstance
The
PKHGet
function inRedis
has appropriate error handling for type and existence checks. However,GetDBInstance
lacks explicit error handling, which could lead to issues if the instance retrieval fails. Consider adding error handling to ensure robustness.
GetDBInstance
insrc/storage/src/storage.cc
lacks error handling.PKHGet
insrc/storage/src/redis_pkhashes.cc
has adequate error handling.Analysis chain
LGTM! Verify error handling.
The function is correctly implemented. Ensure that
GetDBInstance
andPKHGet
handle errors correctly.The code changes are approved.
Run the following script to verify error handling:
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify error handling in `GetDBInstance` and `PKHGet`. # Test: Search for the function implementations. Expect: Proper error handling. rg --type cpp -A 5 $'std::unique_ptr<Redis>& Storage::GetDBInstance' rg --type cpp -A 5 $'Status Redis::PKHGet'Length of output: 1049
Script:
#!/bin/bash # Description: Gather more context around `GetDBInstance` and `PKHGet` for error handling. # Search for the full implementation of `GetDBInstance` to check for error handling. rg --type cpp -A 20 'std::unique_ptr<Redis>& Storage::GetDBInstance' # Search for the full implementation of `PKHGet` to check for error handling. rg --type cpp -A 20 'Status Redis::PKHGet'Length of output: 3083
498-502
: Improve Error Handling inGetDBInstance
The
GetDBInstance
function lacks explicit error handling, which could lead to issues if an invalid index is accessed. Consider adding checks to ensure the index is valid before accessing the instance. ThePKHPersist
function has some error handling mechanisms in place, which is good practice.
- File:
src/storage/src/storage.cc
- Function:
GetDBInstance
Analysis chain
LGTM! Verify error handling.
The function is correctly implemented. Ensure that
GetDBInstance
andPKHPersist
handle errors correctly.The code changes are approved.
Run the following script to verify error handling:
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify error handling in `GetDBInstance` and `PKHPersist`. # Test: Search for the function implementations. Expect: Proper error handling. rg --type cpp -A 5 $'std::unique_ptr<Redis>& Storage::GetDBInstance' rg --type cpp -A 5 $'Status Redis::PKHPersist'Length of output: 1096
Script:
#!/bin/bash # Description: Retrieve complete implementations to verify error handling in `GetDBInstance` and `PKHPersist`. # Search for the complete implementation of `GetDBInstance` to check for error handling. rg --type cpp -A 20 $'std::unique_ptr<Redis>& Storage::GetDBInstance' # Search for the complete implementation of `PKHPersist` to check for error handling. rg --type cpp -A 20 $'Status Redis::PKHPersist'Length of output: 3101
515-518
: Add Error Handling toGetDBInstance
and VerifyRedis::PKHSet
GetDBInstance
should include error handling to manage invalid instance indices.- Ensure comprehensive error handling in
Redis::PKHSet
to cover all possible error conditions.Please review and address these points to improve the robustness of the code.
Analysis chain
LGTM! Verify error handling.
The function is correctly implemented. Ensure that
GetDBInstance
andPKHSet
handle errors correctly.The code changes are approved.
Run the following script to verify error handling:
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify error handling in `GetDBInstance` and `PKHSet`. # Test: Search for the function implementations. Expect: Proper error handling. rg --type cpp -A 5 $'std::unique_ptr<Redis>& Storage::GetDBInstance' rg --type cpp -A 5 $'Status Redis::PKHSet'Length of output: 1054
Script:
#!/bin/bash # Description: Gather more lines to verify error handling in `GetDBInstance` and `Redis::PKHSet`. # Get additional context for `GetDBInstance` rg --type cpp -A 10 $'std::unique_ptr<Redis>& Storage::GetDBInstance(const std::string& key)' # Get additional context for `Redis::PKHSet` rg --type cpp -A 10 $'Status Redis::PKHSet(const Slice& key, const Slice& field, const Slice& value, int32_t* res)'Length of output: 211
Script:
#!/bin/bash # Description: Refine search to verify error handling in `GetDBInstance` and `Redis::PKHSet`. # Search for `GetDBInstance` with more context rg --type cpp -A 15 'std::unique_ptr<Redis>& Storage::GetDBInstance' # Search for `Redis::PKHSet` with more context rg --type cpp -A 15 'Status Redis::PKHSet'Length of output: 2388
src/pika_command.cc (1)
19-19
: Add include guard forpika_pkhash.h
.Ensure that the header file
pika_pkhash.h
is included only once to prevent potential redefinition errors.Consider adding an include guard in
pika_pkhash.h
if it doesn't already exist.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (16)
- include/acl.h (1 hunks)
- include/pika_command.h (6 hunks)
- include/pika_pkhash.h (1 hunks)
- src/pika_client_conn.cc (1 hunks)
- src/pika_command.cc (10 hunks)
- src/pika_pkhash.cc (1 hunks)
- src/storage/include/storage/storage.h (18 hunks)
- src/storage/include/storage/storage_define.h (2 hunks)
- src/storage/src/base_filter.h (6 hunks)
- src/storage/src/base_value_format.h (5 hunks)
- src/storage/src/pkhash_data_value_format.h (1 hunks)
- src/storage/src/redis.cc (12 hunks)
- src/storage/src/redis.h (17 hunks)
- src/storage/src/redis_hashes.cc (37 hunks)
- src/storage/src/redis_pkhashes.cc (1 hunks)
- src/storage/src/storage.cc (36 hunks)
Files skipped from review due to trivial changes (2)
- src/pika_client_conn.cc
- src/storage/src/redis_hashes.cc
Additional comments not posted (147)
src/storage/include/storage/storage_define.h (2)
47-47
: LGTM!The addition of the new enumerator
kPKHashDataCF
is correct and consistent with the existing enumerators.The code changes are approved.
124-132
: LGTM!The formatting changes improve readability without altering the logic of the function.
The code changes are approved.
src/storage/src/base_value_format.h (3)
21-31
: LGTM!The addition of the new enumerators
kPKHashes
andkNones
is correct and consistent with the existing enumerators.The code changes are approved.
34-35
: LGTM!The update to the
DataTypeStrings
array ensures that all enumerators have corresponding string values.The code changes are approved.
52-53
: LGTM!The formatting changes improve readability without altering the logic of the code.
The code changes are approved.
Also applies to: 100-101, 153-153, 163-167
include/pika_pkhash.h (7)
14-38
: LGTM!The class
PKHExpireCmd
is correctly implemented.The code changes are approved.
40-64
: LGTM!The class
PKHExpireatCmd
is correctly implemented.The code changes are approved.
65-89
: LGTM!The class
PKHExpiretimeCmd
is correctly implemented.The code changes are approved.
91-115
: LGTM!The class
PKHPersistCmd
is correctly implemented.The code changes are approved.
117-141
: LGTM!The class
PKHTTLCmd
is correctly implemented.The code changes are approved.
143-164
: LGTM!The class
PKHGetCmd
is correctly implemented.The code changes are approved.
166-187
: LGTM!The class
PKHSetCmd
is correctly implemented.The code changes are approved.
src/pika_pkhash.cc (14)
16-38
: LGTM!The method
PKHExpireCmd::DoInitial
is correctly implemented.The code changes are approved.
40-53
: LGTM!The method
PKHExpireCmd::Do
is correctly implemented.The code changes are approved.
55-80
: LGTM!The method
PKHExpireatCmd::DoInitial
is correctly implemented.The code changes are approved.
81-94
: LGTM!The method
PKHExpireatCmd::Do
is correctly implemented.The code changes are approved.
96-116
: LGTM!The method
PKHExpiretimeCmd::DoInitial
is correctly implemented.The code changes are approved.
117-130
: LGTM!The method
PKHExpiretimeCmd::Do
is correctly implemented.The code changes are approved.
132-150
: LGTM!The method
PKHPersistCmd::DoInitial
is correctly implemented.The code changes are approved.
151-164
: LGTM!The method
PKHPersistCmd::Do
is correctly implemented.The code changes are approved.
166-184
: LGTM!The method
PKHTTLCmd::DoInitial
is correctly implemented.The code changes are approved.
185-198
: LGTM!The method
PKHTTLCmd::Do
is correctly implemented.The code changes are approved.
200-207
: LGTM!The method
PKHGetCmd::DoInitial
is correctly implemented.The code changes are approved.
209-222
: LGTM!The method
PKHGetCmd::Do
is correctly implemented.The code changes are approved.
244-252
: LGTM!The method
PKHSetCmd::DoInitial
is correctly implemented.The code changes are approved.
254-265
: LGTM!The method
PKHSetCmd::Do
is correctly implemented.The code changes are approved.
src/storage/src/base_filter.h (5)
Line range hint
21-74
: LGTM!The class
BaseMetaFilter
is correctly implemented.The code changes are approved.
Line range hint
114-228
: LGTM!The class
BaseDataFilter
is correctly implemented. The constructor reformatting improves readability.The code changes are approved.
250-250
: LGTM!The type alias
PKHashesMetaFilter
is correctly implemented.The code changes are approved.
251-251
: LGTM!The type alias
PKHashesMetaFilterFactory
is correctly implemented.The code changes are approved.
252-252
: LGTM!The type alias
PKHashesDataFilter
is correctly implemented.The code changes are approved.
include/acl.h (1)
55-55
: LGTM!The addition of the
PKHASH
enumerator to theAclCategory
enum is consistent with the PR objectives and summary.The code changes are approved.
include/pika_command.h (14)
141-141
: LGTM!The addition of the
kCmdNamePKHSet
constant is consistent with the PR objectives and summary.The code changes are approved.
142-142
: LGTM!The addition of the
kCmdNamePKHSetex
constant is consistent with the PR objectives and summary.The code changes are approved.
143-143
: LGTM!The addition of the
kCmdNamePKHExpire
constant is consistent with the PR objectives and summary.The code changes are approved.
144-144
: LGTM!The addition of the
kCmdNamePKHExpireat
constant is consistent with the PR objectives and summary.The code changes are approved.
145-145
: LGTM!The addition of the
kCmdNamePKHExpiretime
constant is consistent with the PR objectives and summary.The code changes are approved.
146-146
: LGTM!The addition of the
kCmdNamePKHTTL
constant is consistent with the PR objectives and summary.The code changes are approved.
147-147
: LGTM!The addition of the
kCmdNamePKHPersist
constant is consistent with the PR objectives and summary.The code changes are approved.
148-148
: LGTM!The addition of the
kCmdNamePKHGet
constant is consistent with the PR objectives and summary.The code changes are approved.
149-149
: LGTM!The addition of the
kCmdNamePKHExists
constant is consistent with the PR objectives and summary.The code changes are approved.
150-150
: LGTM!The addition of the
kCmdNamePKHDel
constant is consistent with the PR objectives and summary.The code changes are approved.
151-151
: LGTM!The addition of the
kCmdNamePKHLen
constant is consistent with the PR objectives and summary.The code changes are approved.
152-152
: LGTM!The addition of the
kCmdNamePKHStrlen
constant is consistent with the PR objectives and summary.The code changes are approved.
153-153
: LGTM!The addition of the
kCmdNamePKHIncrby
constant is consistent with the PR objectives and summary.The code changes are approved.
154-154
: LGTM!The addition of the
kCmdNamePKHIncrbyfloat
constant is consistent with the PR objectives and summary.The code changes are approved.
src/storage/src/redis.cc (3)
Line range hint
30-42
: LGTM!The constructor is correctly initializing the new column family options for
pika_hash_data_cf
.The code changes are approved.
102-111
: LGTM!The function is correctly setting up the new column family options for
pika_hash_data_cf
.The code changes are approved.
218-218
: LGTM!The function is correctly including
kPKHashDataCF
in the list of column families to compact.The code changes are approved.
src/storage/src/redis.h (8)
246-248
: LGTM!The function is correctly implemented to retrieve column family handles for PK Hashes.
The code changes are approved.
253-254
: LGTM!The function is correctly implemented to set expiration time for PK Hash fields.
The code changes are approved.
255-256
: LGTM!The function is correctly implemented to set expiration time for PK Hash fields based on a timestamp.
The code changes are approved.
257-258
: LGTM!The function is correctly implemented to retrieve expiration times for PK Hash fields.
The code changes are approved.
259-260
: LGTM!The function is correctly implemented to retrieve TTL for PK Hash fields.
The code changes are approved.
261-262
: LGTM!The function is correctly implemented to make PK Hash fields persistent by removing their expiration.
The code changes are approved.
263-263
: LGTM!The function is correctly implemented to retrieve the value of a PK Hash field.
The code changes are approved.
264-264
: LGTM!The function is correctly implemented to set the value of a PK Hash field.
The code changes are approved.
src/storage/include/storage/storage.h (16)
26-26
: LGTM!The reordering of include statements is acceptable.
The code changes are approved.
28-28
: LGTM!The inclusion of the new header file is necessary for the new functionality.
The code changes are approved.
Line range hint
98-104
: LGTM!The addition of the
operator+
method forKeyInfo
struct is correct and useful for combining key information.The code changes are approved.
111-113
: LGTM!The addition of the
ttl
field toValueStatus
struct is necessary for managing time-to-live information.The code changes are approved.
157-159
: LGTM!The addition of the
Operation
enum is useful for defining various operations in the background task.The code changes are approved.
171-173
: LGTM!The addition of the constructor and destructor for the
Storage
class is necessary for proper initialization and cleanup.The code changes are approved.
256-257
: LGTM!The method signature update ensures consistent formatting and clarity.
The code changes are approved.
262-263
: LGTM!The method signature update ensures consistent formatting and clarity.
The code changes are approved.
272-273
: LGTM!The method signature update ensures consistent formatting and clarity.
The code changes are approved.
482-483
: LGTM!The method signature update ensures consistent formatting and clarity.
The code changes are approved.
506-507
: LGTM!The method signature update ensures consistent formatting and clarity.
The code changes are approved.
516-517
: LGTM!The method signature update ensures consistent formatting and clarity.
The code changes are approved.
590-591
: LGTM!The method signature update ensures consistent formatting and clarity.
The code changes are approved.
753-754
: LGTM!The method signature update ensures consistent formatting and clarity.
The code changes are approved.
1000-1001
: LGTM!The method signature update ensures consistent formatting and clarity.
The code changes are approved.
1035-1036
: LGTM!The method signature update ensures consistent formatting and clarity.
The code changes are approved.
src/storage/src/storage.cc (1)
492-496
: LGTM! Verify error handling.The function is correctly implemented. Ensure that
GetDBInstance
andPKHExpiretime
handle errors correctly.The code changes are approved.
Run the following script to verify error handling:
src/pika_command.cc (73)
486-487
: Correct initialization ofPKHSetCmd
.The command
PKHSetCmd
is correctly initialized and inserted into the command table.The code changes are approved.
490-492
: Correct initialization ofPKHExpireCmd
.The command
PKHExpireCmd
is correctly initialized and inserted into the command table.The code changes are approved.
494-496
: Correct initialization ofPKHExpireatCmd
.The command
PKHExpireatCmd
is correctly initialized and inserted into the command table.The code changes are approved.
498-500
: Correct initialization ofPKHExpiretimeCmd
.The command
PKHExpiretimeCmd
is correctly initialized and inserted into the command table.The code changes are approved.
502-503
: Correct initialization ofPKHTTLCmd
.The command
PKHTTLCmd
is correctly initialized and inserted into the command table.The code changes are approved.
506-508
: Correct initialization ofPKHPersistCmd
.The command
PKHPersistCmd
is correctly initialized and inserted into the command table.The code changes are approved.
510-511
: Correct initialization ofPKHGetCmd
.The command
PKHGetCmd
is correctly initialized and inserted into the command table.The code changes are approved.
56-57
: Consistent formatting forCompactCmd
.The formatting change improves readability by aligning parameters across multiple lines.
The code changes are approved.
60-61
: Consistent formatting forCompactRangeCmd
.The formatting change improves readability by aligning parameters across multiple lines.
The code changes are approved.
80-81
: Consistent formatting forFlushallCmd
.The formatting change improves readability by aligning parameters across multiple lines.
The code changes are approved.
85-86
: Consistent formatting forFlushdbCmd
.The formatting change improves readability by aligning parameters across multiple lines.
The code changes are approved.
159-160
: Consistent formatting forClearCacheCmd
.The formatting change improves readability by aligning parameters across multiple lines.
The code changes are approved.
162-163
: Consistent formatting forLastsaveCmd
.The formatting change improves readability by aligning parameters across multiple lines.
The code changes are approved.
247-248
: Consistent formatting forSetCmd
.The formatting change improves readability by aligning parameters across multiple lines.
The code changes are approved.
251-253
: Consistent formatting forGetCmd
.The formatting change improves readability by aligning parameters across multiple lines.
The code changes are approved.
256-258
: Consistent formatting forDelCmd
.The formatting change improves readability by aligning parameters across multiple lines.
The code changes are approved.
264-265
: Consistent formatting forIncrCmd
.The formatting change improves readability by aligning parameters across multiple lines.
The code changes are approved.
269-269
: Consistent formatting forIncrbyCmd
.The formatting change improves readability by aligning parameters across multiple lines.
The code changes are approved.
273-274
: Consistent formatting forIncrbyfloatCmd
.The formatting change improves readability by aligning parameters across multiple lines.
The code changes are approved.
277-278
: Consistent formatting forDecrCmd
.The formatting change improves readability by aligning parameters across multiple lines.
The code changes are approved.
282-282
: Consistent formatting forDecrbyCmd
.The formatting change improves readability by aligning parameters across multiple lines.
The code changes are approved.
286-286
: Consistent formatting forGetsetCmd
.The formatting change improves readability by aligning parameters across multiple lines.
The code changes are approved.
290-290
: Consistent formatting forAppendCmd
.The formatting change improves readability by aligning parameters across multiple lines.
The code changes are approved.
293-295
: Consistent formatting forMgetCmd
.The formatting change improves readability by aligning parameters across multiple lines.
The code changes are approved.
303-303
: Consistent formatting forSetnxCmd
.The formatting change improves readability by aligning parameters across multiple lines.
The code changes are approved.
306-307
: Consistent formatting forSetexCmd
.The formatting change improves readability by aligning parameters across multiple lines.
The code changes are approved.
310-311
: Consistent formatting forPsetexCmd
.The formatting change improves readability by aligning parameters across multiple lines.
The code changes are approved.
315-315
: Consistent formatting forDelvxCmd
.The formatting change improves readability by aligning parameters across multiple lines.
The code changes are approved.
318-319
: Consistent formatting forMsetCmd
.The formatting change improves readability by aligning parameters across multiple lines.
The code changes are approved.
322-323
: Consistent formatting forMsetnxCmd
.The formatting change improves readability by aligning parameters across multiple lines.
The code changes are approved.
327-328
: Consistent formatting forGetrangeCmd
.The formatting change improves readability by aligning parameters across multiple lines.
The code changes are approved.
332-332
: Consistent formatting forSetrangeCmd
.The formatting change improves readability by aligning parameters across multiple lines.
The code changes are approved.
335-337
: Consistent formatting forStrlenCmd
.The formatting change improves readability by aligning parameters across multiple lines.
The code changes are approved.
340-342
: Consistent formatting forExistsCmd
.The formatting change improves readability by aligning parameters across multiple lines.
The code changes are approved.
346-347
: Consistent formatting forExpireCmd
.The formatting change improves readability by aligning parameters across multiple lines.
The code changes are approved.
351-352
: Consistent formatting forPexpireCmd
.The formatting change improves readability by aligning parameters across multiple lines.
The code changes are approved.
355-357
: Consistent formatting forExpireatCmd
.The formatting change improves readability by aligning parameters across multiple lines.
The code changes are approved.
360-362
: Consistent formatting forPexpireatCmd
.The formatting change improves readability by aligning parameters across multiple lines.
The code changes are approved.
365-366
: Consistent formatting forTtlCmd
.The formatting change improves readability by aligning parameters across multiple lines.
The code changes are approved.
369-370
: Consistent formatting forPttlCmd
.The formatting change improves readability by aligning parameters across multiple lines.
The code changes are approved.
373-375
: Consistent formatting forPersistCmd
.The formatting change improves readability by aligning parameters across multiple lines.
The code changes are approved.
378-379
: Consistent formatting forTypeCmd
.The formatting change improves readability by aligning parameters across multiple lines.
The code changes are approved.
391-391
: Consistent formatting forPKSetexAtCmd
.The formatting change improves readability by aligning parameters across multiple lines.
The code changes are approved.
394-395
: Consistent formatting forPKScanRangeCmd
.The formatting change improves readability by aligning parameters across multiple lines.
The code changes are approved.
398-399
: Consistent formatting forPKRScanRangeCmd
.The formatting change improves readability by aligning parameters across multiple lines.
The code changes are approved.
404-405
: Consistent formatting forHDelCmd
.The formatting change improves readability by aligning parameters across multiple lines.
The code changes are approved.
408-409
: Consistent formatting forHSetCmd
.The formatting change improves readability by aligning parameters across multiple lines.
The code changes are approved.
412-414
: Consistent formatting forHGetCmd
.The formatting change improves readability by aligning parameters across multiple lines.
The code changes are approved.
417-419
: Consistent formatting forHGetallCmd
.The formatting change improves readability by aligning parameters across multiple lines.
The code changes are approved.
422-424
: Consistent formatting forHExistsCmd
.The formatting change improves readability by aligning parameters across multiple lines.
The code changes are approved.
427-428
: Consistent formatting forHIncrbyCmd
.The formatting change improves readability by aligning parameters across multiple lines.
The code changes are approved.
431-433
: Consistent formatting forHIncrbyfloatCmd
.The formatting change improves readability by aligning parameters across multiple lines.
The code changes are approved.
436-438
: Consistent formatting forHKeysCmd
.The formatting change improves readability by aligning parameters across multiple lines.
The code changes are approved.
441-443
: Consistent formatting forHLenCmd
.The formatting change improves readability by aligning parameters across multiple lines.
The code changes are approved.
446-448
: Consistent formatting forHMgetCmd
.The formatting change improves readability by aligning parameters across multiple lines.
The code changes are approved.
451-452
: Consistent formatting forHMsetCmd
.The formatting change improves readability by aligning parameters across multiple lines.
The code changes are approved.
455-456
: Consistent formatting forHSetnxCmd
.The formatting change improves readability by aligning parameters across multiple lines.
The code changes are approved.
459-461
: Consistent formatting forHStrlenCmd
.The formatting change improves readability by aligning parameters across multiple lines.
The code changes are approved.
464-466
: Consistent formatting forHValsCmd
.The formatting change improves readability by aligning parameters across multiple lines.
The code changes are approved.
469-470
: Consistent formatting forHScanCmd
.The formatting change improves readability by aligning parameters across multiple lines.
The code changes are approved.
473-474
: Consistent formatting forHScanxCmd
.The formatting change improves readability by aligning parameters across multiple lines.
The code changes are approved.
477-478
: Consistent formatting forPKHScanRangeCmd
.The formatting change improves readability by aligning parameters across multiple lines.
The code changes are approved.
481-482
: Consistent formatting forPKHRScanRangeCmd
.The formatting change improves readability by aligning parameters across multiple lines.
The code changes are approved.
514-516
: Consistent formatting forLIndexCmd
.The formatting change improves readability by aligning parameters across multiple lines.
The code changes are approved.
518-519
: Consistent formatting forLInsertCmd
.The formatting change improves readability by aligning parameters across multiple lines.
The code changes are approved.
522-524
: Consistent formatting forLLenCmd
.The formatting change improves readability by aligning parameters across multiple lines.
The code changes are approved.
526-527
: Consistent formatting forBLPopCmd
.The formatting change improves readability by aligning parameters across multiple lines.
The code changes are approved.
530-531
: Consistent formatting forLPopCmd
.The formatting change improves readability by aligning parameters across multiple lines.
The code changes are approved.
535-535
: Consistent formatting forLPushCmd
.The formatting change improves readability by aligning parameters across multiple lines.
The code changes are approved.
538-539
: Consistent formatting forLPushxCmd
.The formatting change improves readability by aligning parameters across multiple lines.
The code changes are approved.
543-544
: Consistent formatting forLRangeCmd
.The formatting change improves readability by aligning parameters across multiple lines.
The code changes are approved.
546-547
: Consistent formatting forLRemCmd
.The formatting change improves readability by aligning parameters across multiple lines.
The code changes are approved.
549-550
: **Consistent formatting for
class PKHashDataValue : public InternalValue { | ||
public: | ||
/* | ||
* The header of the Value field is initially initialized to knulltype | ||
*/ | ||
explicit PKHashDataValue(const rocksdb::Slice& user_value) : InternalValue(DataType::kNones, user_value) {} | ||
virtual ~PKHashDataValue() {} | ||
|
||
virtual rocksdb::Slice Encode() { | ||
size_t usize = user_value_.size(); | ||
size_t needed = usize + kSuffixReserveLength + kTimestampLength * 2; | ||
char* dst = ReAllocIfNeeded(needed); | ||
char* start_pos = dst; | ||
|
||
memcpy(dst, user_value_.data(), user_value_.size()); | ||
dst += user_value_.size(); | ||
memcpy(dst, reserve_, kSuffixReserveLength); | ||
dst += kSuffixReserveLength; | ||
EncodeFixed64(dst, ctime_); | ||
dst += kTimestampLength; | ||
EncodeFixed64(dst, etime_); | ||
dst += kTimestampLength; // todo(DDD) 待确认,看这个是否需要。 | ||
|
||
return rocksdb::Slice(start_pos, needed); | ||
} | ||
|
||
private: | ||
const size_t kDefaultValueSuffixLength = kSuffixReserveLength + kTimestampLength * 2; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! But address the TODO comment.
The PKHashDataValue
class is correctly implemented. However, the TODO comment in the Encode
method needs to be addressed.
The code changes are approved.
Do you want me to help address the TODO comment or open a GitHub issue to track this task?
class ParsedPKHashDataValue : public ParsedInternalValue { | ||
public: | ||
// Use this constructor after rocksdb::DB::Get(), since we use this in | ||
// the implement of user interfaces and may need to modify the | ||
// original value suffix, so the value_ must point to the string | ||
explicit ParsedPKHashDataValue(std::string* value) : ParsedInternalValue(value) { | ||
if (value_->size() >= kPKHashDataValueSuffixLength) { | ||
user_value_ = rocksdb::Slice(value_->data(), value_->size() - kPKHashDataValueSuffixLength); | ||
memcpy(reserve_, value_->data() + user_value_.size(), kSuffixReserveLength); | ||
ctime_ = DecodeFixed64(value_->data() + user_value_.size() + kSuffixReserveLength); | ||
etime_ = DecodeFixed64(value_->data() + user_value_.size() + kSuffixReserveLength + kTimestampLength); | ||
} | ||
} | ||
|
||
// Use this constructor in rocksdb::CompactionFilter::Filter(), | ||
// since we use this in Compaction process, all we need to do is parsing | ||
// the rocksdb::Slice, so don't need to modify the original value, value_ can be | ||
// set to nullptr | ||
explicit ParsedPKHashDataValue(const rocksdb::Slice& value) : ParsedInternalValue(value) { | ||
if (value.size() >= kPKHashDataValueSuffixLength) { | ||
user_value_ = rocksdb::Slice(value.data(), value.size() - kPKHashDataValueSuffixLength); | ||
memcpy(reserve_, value.data() + user_value_.size(), kSuffixReserveLength); | ||
ctime_ = DecodeFixed64(value.data() + user_value_.size() + kSuffixReserveLength); | ||
etime_ = DecodeFixed64(value_->data() + user_value_.size() + kSuffixReserveLength + kTimestampLength); | ||
} | ||
} | ||
|
||
virtual ~ParsedPKHashDataValue() = default; | ||
|
||
void SetEtimeToValue() override { | ||
if (value_) { | ||
char* dst = const_cast<char*>(value_->data()) + value_->size() - kTimestampLength; | ||
EncodeFixed64(dst, etime_); | ||
} | ||
} | ||
|
||
void SetCtimeToValue() override { | ||
if (value_) { | ||
char* dst = const_cast<char*>(value_->data()) + value_->size() - kTimestampLength - kTimestampLength; | ||
EncodeFixed64(dst, ctime_); | ||
} | ||
} | ||
|
||
void SetReserveToValue() { | ||
if (value_) { | ||
char* dst = const_cast<char*>(value_->data()) + value_->size() - kPKHashDataValueSuffixLength; | ||
memcpy(dst, reserve_, kSuffixReserveLength); | ||
} | ||
} | ||
|
||
virtual void StripSuffix() override { | ||
if (value_) { | ||
value_->erase(value_->size() - kPKHashDataValueSuffixLength, kPKHashDataValueSuffixLength); | ||
} | ||
} | ||
|
||
void SetTimestamp(int64_t timestamp) { | ||
etime_ = timestamp; | ||
SetEtimeToValue(); | ||
} | ||
|
||
protected: | ||
virtual void SetVersionToValue() override {}; | ||
|
||
private: | ||
const size_t kPKHashDataValueSuffixLength = kSuffixReserveLength + kTimestampLength * 2; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! But address the empty SetVersionToValue
method.
The ParsedPKHashDataValue
class is correctly implemented. However, the SetVersionToValue
method is empty and might need to be implemented.
The code changes are approved.
Do you want me to help implement the SetVersionToValue
method or open a GitHub issue to track this task?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 4
Outside diff range and nitpick comments (1)
src/storage/tests/pkhashes_test.cc (1)
20-20
: Avoid using 'using namespace' in the global scopeUsing
using namespace storage;
in the global scope can lead to potential name collisions and reduces code maintainability. It's generally better to avoid importing entire namespaces into the global scope. Instead, consider prefixing withstorage::
or using the namespace within a limited scope.
std::map<storage::DataType, rocksdb::Status> type_status; | ||
db.Expire("B_HKEYS_KEY", 1); | ||
ASSERT_TRUE(type_status[storage::DataType::kHashes].ok()); | ||
std::this_thread::sleep_for(std::chrono::milliseconds(2000)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Incorrect error checking using uninitialized type_status
in HKeys
test
In the HKeys
test, the type_status
map is declared but not used. The assertion ASSERT_TRUE(type_status[storage::DataType::kHashes].ok());
is invalid because type_status
is empty and uninitialized.
Apply this diff to correct the error checking:
-std::map<storage::DataType, rocksdb::Status> type_status;
db.Expire("B_HKEYS_KEY", 1);
-ASSERT_TRUE(type_status[storage::DataType::kHashes].ok());
+ASSERT_TRUE(s.ok());
If db.Expire
returns a status, modify the code to capture and assert it:
-std::map<storage::DataType, rocksdb::Status> type_status;
-storage::Status s = db.Expire("B_HKEYS_KEY", 1);
+storage::Status s = db.Expire("B_HKEYS_KEY", 1);
ASSERT_TRUE(s.ok());
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
std::map<storage::DataType, rocksdb::Status> type_status; | |
db.Expire("B_HKEYS_KEY", 1); | |
ASSERT_TRUE(type_status[storage::DataType::kHashes].ok()); | |
std::this_thread::sleep_for(std::chrono::milliseconds(2000)); | |
storage::Status s = db.Expire("B_HKEYS_KEY", 1); | |
ASSERT_TRUE(s.ok()); | |
std::this_thread::sleep_for(std::chrono::milliseconds(2000)); |
void SetUp() override { | ||
std::string path = "./db/pkhashes"; | ||
pstd::DeleteDirIfExist(path); | ||
mkdir(path.c_str(), 0755); | ||
storage_options.options.create_if_missing = true; | ||
s = db.Open(storage_options, path); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check the return status of db.Open
in SetUp()
The return status of db.Open(storage_options, path);
is assigned to s
, but there is no check to verify if the operation was successful. Ignoring the status may lead to unexpected behavior if the database fails to open.
Apply this diff to add an assertion to ensure the database opens successfully:
s = db.Open(storage_options, path);
+ASSERT_TRUE(s.ok());
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
void SetUp() override { | |
std::string path = "./db/pkhashes"; | |
pstd::DeleteDirIfExist(path); | |
mkdir(path.c_str(), 0755); | |
storage_options.options.create_if_missing = true; | |
s = db.Open(storage_options, path); | |
} | |
void SetUp() override { | |
std::string path = "./db/pkhashes"; | |
pstd::DeleteDirIfExist(path); | |
mkdir(path.c_str(), 0755); | |
storage_options.options.create_if_missing = true; | |
s = db.Open(storage_options, path); | |
ASSERT_TRUE(s.ok()); | |
} |
std::map<storage::DataType, rocksdb::Status> type_status; | ||
int ret = db->Expire(key, 1); | ||
if ((ret == 0) || !type_status[storage::DataType::kHashes].ok()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unused variable type_status
and incorrect error checking in make_expired
The variable type_status
is declared but not used effectively. The condition !type_status[storage::DataType::kHashes].ok()
is invalid because type_status
is empty and uninitialized. This leads to improper error handling in the make_expired
function.
Apply this diff to remove the unused variable and correct the error checking:
-static std::map<storage::DataType, rocksdb::Status> type_status;
int ret = db->Expire(key, 1);
-if ((ret == 0) || !type_status[storage::DataType::kHashes].ok()) {
+if (ret == 0) {
return false;
}
Alternatively, if you need to check the status returned by Expire
, modify the code as follows:
-static std::map<storage::DataType, rocksdb::Status> type_status;
-int ret = db->Expire(key, 1);
+storage::Status s = db->Expire(key, 1);
-if ((ret == 0) || !type_status[storage::DataType::kHashes].ok()) {
+if (!s.ok()) {
return false;
}
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
std::map<storage::DataType, rocksdb::Status> type_status; | |
int ret = db->Expire(key, 1); | |
if ((ret == 0) || !type_status[storage::DataType::kHashes].ok()) { | |
int ret = db->Expire(key, 1); | |
if (ret == 0) { | |
``` | |
This suggestion removes the unused `type_status` variable and simplifies the condition to check only the return value of `db->Expire(key, 1)`. | |
Note: The second diff snippet provided in the review comment is also valid and could be used as an alternative solution. If you prefer that approach, the suggestion would be: | |
```suggestion | |
storage::Status s = db->Expire(key, 1); | |
if (!s.ok()) { |
std::map<storage::DataType, rocksdb::Status> type_status; | ||
db.Expire("HDEL_TIMEOUT_KEY", 1); | ||
ASSERT_TRUE(type_status[storage::DataType::kHashes].ok()); | ||
std::this_thread::sleep_for(std::chrono::milliseconds(2000)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Incorrect error checking using uninitialized type_status
in HDel
test
In the HDel
test, the type_status
map is declared but not populated. The assertion ASSERT_TRUE(type_status[storage::DataType::kHashes].ok());
will not function correctly since type_status
is empty, leading to invalid error checking.
Apply this diff to correct the error checking:
-std::map<storage::DataType, rocksdb::Status> type_status;
db.Expire("HDEL_TIMEOUT_KEY", 1);
-ASSERT_TRUE(type_status[storage::DataType::kHashes].ok());
+ASSERT_TRUE(s.ok());
If db.Expire
returns a status, capture and check it:
-std::map<storage::DataType, rocksdb::Status> type_status;
-storage::Status s = db.Expire("HDEL_TIMEOUT_KEY", 1);
+storage::Status s = db.Expire("HDEL_TIMEOUT_KEY", 1);
ASSERT_TRUE(s.ok());
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
std::map<storage::DataType, rocksdb::Status> type_status; | |
db.Expire("HDEL_TIMEOUT_KEY", 1); | |
ASSERT_TRUE(type_status[storage::DataType::kHashes].ok()); | |
std::this_thread::sleep_for(std::chrono::milliseconds(2000)); | |
storage::Status s = db.Expire("HDEL_TIMEOUT_KEY", 1); | |
ASSERT_TRUE(s.ok()); | |
std::this_thread::sleep_for(std::chrono::milliseconds(2000)); |
Summary by CodeRabbit
New Features
PKHASH
to enhance access control functionality.PKHSet
,PKHGet
, and expiration commands.Bug Fixes
Documentation