feat: a new strategy for auto compaction #2816
Conversation
WalkthroughThe recent changes introduce enhancements to the Pika database's configuration and compaction strategies. New parameters for managing the compaction process have been added to the configuration files, while modifications to various classes enhance functionality and introduce new methods for compaction. Additionally, a new table properties collector is implemented for improved data management in the Redis storage system. These changes collectively improve data handling capabilities and extend the adaptability of the Pika server. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant PikaServer
participant Database
participant Storage
User->>PikaServer: Request data operation
PikaServer->>Database: Check operation type
alt Compact using new strategy
PikaServer->>Storage: Invoke LongestNotCompactiontSstCompact
Storage->>Storage: Perform compaction
else Regular operation
PikaServer->>Database: Execute standard operation
end
Database-->>PikaServer: Return results
PikaServer-->>User: Send final response
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (3)
Files skipped from review as they are similar to previous changes (3)
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: 1
Outside diff range, codebase verification and nitpick comments (1)
src/storage/src/redis.h (1)
109-111: Issues found in the function usage ofLongestNotCompactiontSstCompact.The function
LongestNotCompactiontSstCompactis defined with three parameters insrc/storage/src/redis.h. However, its usage in the following files does not match the definition, as they are missing the third parameter:
src/pika_db.ccsrc/pika_server.ccPlease update the function calls to match the definition.
Analysis chain
LGTM! But verify the function usage in the codebase.
The
LongestNotCompactiontSstCompactmethod is designed to accept aDataTypeoption and a vector ofStatusobjects to store results. The code changes are approved.However, ensure that all function calls to
LongestNotCompactiontSstCompactare correctly implemented.
Verify the function usage of
LongestNotCompactiontSstCompactto ensure consistency with its definition.The function
LongestNotCompactiontSstCompactis used in multiple files, but there seems to be a discrepancy in the number of parameters passed during its usage. Verify that all calls to this function match its definition.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `LongestNotCompactiontSstCompact` are correctly implemented. # Test: Search for the function usage. Expect: Correct implementation. rg --type cpp -A 5 $'LongestNotCompactiontSstCompact'Length of output: 5488
Script:
#!/bin/bash # Description: Verify the function definition and its usage to ensure consistency in parameters. # Search for the function definition in header files rg --type cpp --files-with-matches --glob '*redis.h' 'LongestNotCompactiontSstCompact' | xargs rg --type cpp -A 5 'LongestNotCompactiontSstCompact' # Search for the function usage in source files rg --type cpp --files-with-matches --glob '*pika_db.cc' 'LongestNotCompactiontSstCompact' | xargs rg --type cpp -A 5 'LongestNotCompactiontSstCompact' rg --type cpp --files-with-matches --glob '*pika_server.cc' 'LongestNotCompactiontSstCompact' | xargs rg --type cpp -A 5 'LongestNotCompactiontSstCompact' rg --type cpp --files-with-matches --glob '*storage.cc' 'LongestNotCompactiontSstCompact' | xargs rg --type cpp -A 5 'LongestNotCompactiontSstCompact'Length of output: 2090
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (12)
- conf/pika.conf (1 hunks)
- conf/s_pika.conf (1 hunks)
- include/pika_conf.h (3 hunks)
- include/pika_db.h (1 hunks)
- include/pika_server.h (1 hunks)
- src/pika_conf.cc (3 hunks)
- src/pika_db.cc (1 hunks)
- src/pika_server.cc (4 hunks)
- src/storage/include/storage/storage.h (4 hunks)
- src/storage/src/redis.cc (2 hunks)
- src/storage/src/redis.h (2 hunks)
- src/storage/src/storage.cc (5 hunks)
Additional comments not posted (99)
conf/s_pika.conf (54)
3-3: Specify the port number.Ensure that the port number
9221does not conflict with other services running on the same machine.
4-4: Optimize thread usage.The
thread-numis set to8. Ensure this value is optimal for your server's hardware capabilities.
5-5: Ensure log path exists.Verify that the
log-pathdirectory./log/exists and is writable by the Pika process.
7-7: Ensure database path exists.Verify that the
db-pathdirectory./db/exists and is writable by the Pika process.
8-8: Optimize write buffer size.The
write-buffer-sizeis set to256M. Ensure this value is optimal for your workload and server memory.
9-9: Review timeout setting.The
timeoutis set to30seconds. Ensure this value is appropriate for your use case.
15-15: Review dump expiration setting.The
dump-expireis set to1day. Ensure this value is appropriate for your data retention policy.
18-18: Ensure dump path exists.Verify that the
dump-pathdirectory./dump/block_cache_sizeexists and is writable by the Pika process.
20-20: Review target file size base.The
target-file-size-baseis set to20971520bytes (20 MB). Ensure this value is optimal for your workload.
21-21: Review log expiration setting.The
expire-logs-daysis set to7days. Ensure this value is appropriate for your log retention policy.
22-22: Review log expiration number setting.The
expire-logs-numsis set to300. Ensure this value is appropriate for your log retention policy.
23-23: Review root connection number.The
root-connection-numis set to10. Ensure this value is appropriate for your use case.
24-24: Review slow log threshold.The
slowlog-log-slower-thanis set to100000microseconds (100 ms). Ensure this value is appropriate for your performance monitoring.
25-25: Review binlog file size.The
binlog-file-sizeis set to104857600bytes (100 MB). Ensure this value is optimal for your workload.
26-26: Review compression setting.The
compressionis set tosnappy. Ensure this compression algorithm is appropriate for your use case.
28-28: Review DB sync speed.The
db-sync-speedis set to60MB/s. Ensure this value is optimal for your network and workload.
30-30: Review small compaction threshold.The
small-compaction-thresholdis set to5000. Ensure this value is optimal for your workload.
31-31: Review max write buffer size.The
max-write-buffer-sizeis set to20737418240bytes (20 GB). Ensure this value is optimal for your server memory.
32-32: Review max cache files.The
max-cache-filesis set to8000. Ensure this value is optimal for your workload.
33-33: Review replication number.The
replication-numis set to0. Ensure this value is appropriate for your replication strategy.
34-34: Review consensus level.The
consensus-levelis set to0. Ensure this value is appropriate for your consensus strategy.
35-35: Review max cache statistic keys.The
max-cache-statistic-keysis set to0. Ensure this value is appropriate for your cache statistics.
36-36: Review thread pool size.The
thread-pool-sizeis set to30. Ensure this value is optimal for your server's hardware capabilities.
38-38: Review default slot number.The
default-slot-numis set to1024. Ensure this value is appropriate for your slot management.
39-39: Review instance mode.The
instance-modeis set toclassic. Ensure this value is appropriate for your deployment strategy.
40-40: Review number of databases.The
databasesis set to1. Ensure this value is appropriate for your use case.
41-41: Review sync thread number.The
sync-thread-numis set to1. Ensure this value is optimal for your workload.
42-42: Review arena block size.The
arena-block-sizeis set to33554432bytes (32 MB). Ensure this value is optimal for your workload.
43-43: Review max background jobs.The
max-background-jobsis set to12. Ensure this value is optimal for your server's hardware capabilities.
44-44: Review max background flushes.The
max-background-flushesis set to3. Ensure this value is optimal for your workload.
45-45: Review max background compactions.The
max-background-compactionsis set to9. Ensure this value is optimal for your workload.
46-46: Review rate limiter bandwidth.The
rate-limiter-bandwidthis set to1099511627776bytes (1 TB). Ensure this value is optimal for your network and workload.
47-47: Review number of DB instances.The
db-instance-numis set to1. Ensure this value is appropriate for your use case.
48-48: Review block size.The
block-sizeis set to4096bytes (4 KB). Ensure this value is optimal for your workload.
50-50: Review block cache size.The
block-cacheis set to4294967296bytes (4 GB). Ensure this value is optimal for your server memory.
51-51: Review max subcompactions.The
max-subcompactionsis set to8. Ensure this value is optimal for your workload.
54-54: Review cache LFU decay time.The
cache-lfu-decay-timeis set to1. Ensure this value is appropriate for your cache eviction policy.
55-55: Review cache max memory samples.The
cache-maxmemory-samplesis set to5. Ensure this value is optimal for your cache management.
56-56: Review cache max memory policy.The
cache-maxmemory-policyis set to1. Ensure this value is appropriate for your cache eviction policy.
57-57: Review number of caches.The
cache-numis set to8. Ensure this value is optimal for your workload.
58-58: Review cache model.The
cache-modelis set to0. Ensure this value is appropriate for your cache strategy.
59-59: Review ZSET cache field number per key.The
zset-cache-field-num-per-keyis set to512. Ensure this value is optimal for your workload.
60-60: Review ZSET cache start direction.The
zset-cache-start-directionis set to0. Ensure this value is appropriate for your ZSET cache strategy.
63-63: Review share block cache setting.The
share-block-cacheis set toyes. Ensure this value is appropriate for your cache strategy.
64-64: Review throttle bytes per second.The
throttle-bytes-per-secondis set to102400000bytes (100 MB). Ensure this value is optimal for your network and workload.
65-65: Review max rsync parallel number.The
max-rsync-parallel-numis set to4. Ensure this value is optimal for your workload.
66-66: Review write binlog setting.The
write-binlogis set tono. Ensure this value is appropriate for your replication strategy.
67-67: Review slot migration setting.The
slotmigrateis set tono. Ensure this value is appropriate for your slot management strategy.
73-73: Review compaction strategy.The
compaction-strategyis set toobd-compact. Ensure this value is optimal for your workload and compaction needs.
78-78: Review compact every number of files setting.The
compact-every-num-of-filesis set to10. Ensure this value is optimal for your workload.
86-86: Review force compact file age seconds.The
force-compact-file-age-secondsis set to300seconds (5 minutes). Ensure this value is optimal for your workload.
91-91: Review force compact minimum delete ratio.The
force-compact-min-delete-ratiois set to10. Ensure this value is optimal for your workload.
96-96: Review don't compact SST created in seconds.The
dont-compact-sst-created-in-secondsis set to20. Ensure this value is optimal for your workload.
101-101: Review best delete minimum ratio.The
best-delete-min-ratiois set to20. Ensure this value is optimal for your workload.include/pika_server.h (1)
62-62: Enumeration value addition looks good.The new enumeration value
kCompactOldestOrBestDeleteRatioSstis consistent with the existing naming conventions.src/pika_db.cc (2)
169-175: Verify the impact of commenting outDB::FullCompact.The
FullCompactmethod has been commented out. Ensure that this change does not affect other parts of the codebase that might rely on this method.Verification successful
Verification successful: Commenting out
DB::FullCompactdoes not impact other parts of the codebase.The
DB::FullCompactmethod is not used elsewhere in the codebase, confirming that commenting it out does not affect other parts of the project.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify if `DB::FullCompact` is used elsewhere in the codebase. # Test: Search for the usage of `DB::FullCompact`. Expect: No usage. rg --type cpp -A 5 $'DB::FullCompact'Length of output: 262
177-183: LGTM! But verify the function usage in the codebase.The
LongestNotCompactiontSstCompactmethod ensures thread safety and checks if the database is opened before performing a specific type of data compaction. The code changes are approved.However, ensure that all function calls to
LongestNotCompactiontSstCompactare correctly implemented.src/storage/src/redis.cc (2)
204-260: LGTM! But verify the function usage in the codebase.The
SelectColumnFamilyHandlesmethod determines which column family handles should be selected based on the providedDataTypeandColumnFamilyType. The code changes are approved.However, ensure that all function calls to
SelectColumnFamilyHandlesare correctly implemented.Verification successful
Verification successful!
The
SelectColumnFamilyHandlesfunction is used correctly in the codebase. The only occurrence is insrc/storage/src/redis.cc, confirming that it is implemented as intended.
src/storage/src/redis.cc: Verified usage.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `SelectColumnFamilyHandles` are correctly implemented. # Test: Search for the function usage. Expect: Correct implementation. rg --type cpp -A 5 $'SelectColumnFamilyHandles'Length of output: 791
Script:
#!/bin/bash # Description: Search for all occurrences of `SelectColumnFamilyHandles` in the codebase to confirm there are no other usages that need to be verified. # Test: Search for the function usage. Expect: Correct implementation. rg 'SelectColumnFamilyHandles' --type cppLength of output: 243
262-409: LGTM! But verify the function usage in the codebase.The
LongestNotCompactiontSstCompactmethod implements a complex logic for managing compaction within the Redis storage system. The code changes are approved.However, ensure that all function calls to
LongestNotCompactiontSstCompactare correctly implemented.Verification successful
Function usage verified successfully.
The function
LongestNotCompactiontSstCompactis correctly implemented and used across the codebase.
src/pika_db.ccsrc/pika_server.ccsrc/storage/src/storage.ccsrc/storage/src/redis.hsrc/storage/include/storage/storage.hinclude/pika_db.hScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `LongestNotCompactiontSstCompact` are correctly implemented. # Test: Search for the function usage. Expect: Correct implementation. rg --type cpp -A 5 $'LongestNotCompactiontSstCompact'Length of output: 5488
Script:
#!/bin/bash # Description: Inspect specific lines in the files where `LongestNotCompactiontSstCompact` is used. # Check the usage in src/pika_db.cc rg --type cpp -A 5 -B 5 'LongestNotCompactiontSstCompact' src/pika_db.cc # Check the usage in src/pika_server.cc rg --type cpp -A 5 -B 5 'LongestNotCompactiontSstCompact' src/pika_server.cc # Check the usage in src/storage/src/storage.cc rg --type cpp -A 5 -B 5 'LongestNotCompactiontSstCompact' src/storage/src/storage.cc # Check the usage in src/storage/src/redis.h rg --type cpp -A 5 -B 5 'LongestNotCompactiontSstCompact' src/storage/src/redis.h # Check the usage in src/storage/include/storage/storage.h rg --type cpp -A 5 -B 5 'LongestNotCompactiontSstCompact' src/storage/include/storage/storage.h # Check the usage in include/pika_db.h rg --type cpp -A 5 -B 5 'LongestNotCompactiontSstCompact' include/pika_db.hLength of output: 4372
src/storage/src/redis.h (1)
483-483: LGTM! But verify the variable usage in the codebase.The
in_compact_flag_member variable is likely used to manage the state of compaction processes and ensure thread safety. The code changes are approved.However, ensure that all usages of
in_compact_flag_are correctly implemented.Verification successful
Verify the correct implementation of
in_compact_flag_usage in the compaction process.The variable
in_compact_flag_is used inredis.ccwith atomic operations and aDEFERmacro. This appears to manage the state of a compaction process. Ensure the surrounding code correctly handles this process.
The usage of
in_compact_flag_is correctly implemented.The
in_compact_flag_variable is used to manage the state of the compaction process with appropriate atomic operations and aDEFERmacro to reset the flag. The surrounding code correctly handles the compaction process.
src/storage/src/redis.cc: Verified the correct usage ofin_compact_flag_.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all usages of `in_compact_flag_` are correctly implemented. # Test: Search for the variable usage. Expect: Correct implementation. rg --type cpp -A 5 $'in_compact_flag_'Length of output: 1114
Script:
#!/bin/bash # Description: Verify the context of `in_compact_flag_` usage in `redis.cc`. # Search for the function or method where `in_compact_flag_` is used to understand the complete context. rg --type cpp -A 20 'in_compact_flag_' src/storage/src/redis.ccLength of output: 1025
conf/pika.conf (6)
641-645: LGTM! New compaction strategy parameter.The
compaction-strategyparameter allows users to select betweenfull-compactandobd-compact. The default value isobd-compact.
647-650: LGTM! New parameter for SST file compaction trigger.The
compact-every-num-of-filesparameter specifies the number of SST files that will trigger a compaction event. The default value is set to 10.
652-658: LGTM! New parameter for file age compaction trigger.The
force-compact-file-age-secondsparameter sets a threshold for file age before compaction is forced. The default value is set to 300 seconds.
660-663: LGTM! New parameter for minimum delete ratio compaction trigger.The
force-compact-min-delete-ratioparameter specifies the minimum delete ratio required to force a compaction event. The default value is set to 10.
665-668: LGTM! New parameter for avoiding compaction of newly created SST files.The
dont-compact-sst-created-in-secondsparameter sets a threshold for the minimum age of SST files to avoid compaction. The default value is set to 600 seconds.
670-673: LGTM! New parameter for best delete ratio compaction trigger.The
best-delete-min-ratioparameter specifies the best delete ratio required to trigger a compaction event. The default value is set to 10.include/pika_conf.h (7)
31-35: LGTM! New enumeration for compaction strategies.The
CompactionStrategyenumeration defines two strategies:FullCompactandOldestOrBestDeleteRatioSstCompact.
123-126: LGTM! New accessor method forcompact_every_num_of_files_.The
compact_every_num_of_files()method provides thread-safe access to thecompact_every_num_of_files_member variable.
127-130: LGTM! New accessor method forforce_compact_file_age_seconds_.The
force_compact_file_age_seconds()method provides thread-safe access to theforce_compact_file_age_seconds_member variable.
131-134: LGTM! New accessor method forforce_compact_min_delete_ratio_.The
force_compact_min_delete_ratio()method provides thread-safe access to theforce_compact_min_delete_ratio_member variable.
135-138: LGTM! New accessor method fordont_compact_sst_created_in_seconds_.The
dont_compact_sst_created_in_seconds()method provides thread-safe access to thedont_compact_sst_created_in_seconds_member variable.
139-142: LGTM! New accessor method forbest_delete_min_ratio_.The
best_delete_min_ratio()method provides thread-safe access to thebest_delete_min_ratio_member variable.
143-146: LGTM! New accessor method forcompaction_strategy_.The
compaction_strategy()method provides thread-safe access to thecompaction_strategy_member variable.src/pika_conf.cc (12)
304-307: Ensure minimum value forcompact-every-num-of-files.The code enforces a minimum value of 10 for
compact-every-num-of-files. This is a good practice to prevent misconfigurations.
309-312: Ensure minimum value forforce-compact-file-age-seconds.The code enforces a minimum value of 300 seconds for
force-compact-file-age-seconds. This is a good practice to prevent misconfigurations.
314-317: Ensure minimum value forforce-compact-min-delete-ratio.The code enforces a minimum value of 10 for
force-compact-min-delete-ratio. This is a good practice to prevent misconfigurations.
319-322: Ensure minimum value fordont-compact-sst-created-in-seconds.The code enforces a minimum value of 600 seconds for
dont-compact-sst-created-in-seconds. This is a good practice to prevent misconfigurations.
324-327: Ensure minimum value forbest-delete-min-ratio.The code enforces a minimum value of 10 for
best-delete-min-ratio. This is a good practice to prevent misconfigurations.
329-337: Validate compaction strategy.The code correctly sets the compaction strategy based on the
compaction-strategyparameter, defaulting toFullCompactif the value is not recognized.
820-823: Ensure minimum value forcompact-every-num-of-files.The code enforces a minimum value of 1 for
compact-every-num-of-files. This is a good practice to prevent misconfigurations.
824-827: Ensure minimum value forforce-compact-file-age-seconds.The code enforces a minimum value of 300 seconds for
force-compact-file-age-seconds. This is a good practice to prevent misconfigurations.
828-831: Ensure minimum value forforce-compact-min-delete-ratio.The code enforces a minimum value of 5 for
force-compact-min-delete-ratio. This is a good practice to prevent misconfigurations.
832-835: Ensure minimum value fordont-compact-sst-created-in-seconds.The code enforces a minimum value of 300 seconds for
dont-compact-sst-created-in-seconds. This is a good practice to prevent misconfigurations.
836-839: Ensure minimum value forbest-delete-min-ratio.The code enforces a minimum value of 10 for
best-delete-min-ratio. This is a good practice to prevent misconfigurations.
841-849: Validate compaction strategy.The code correctly sets the compaction strategy based on the
compaction-strategyparameter, defaulting toFullCompactif the value is not recognized.src/storage/include/storage/storage.h (5)
74-81: Addition ofCompactParamstruct looks good.The
CompactParamstruct is well-defined and its fields are appropriately typed.
82-82: Addition ofcompact_param_field looks good.The
compact_param_field is logically added and aligns with the newCompactParamstruct.
165-166: Addition ofkCompactOldestOrBestDeleteRatioSsttoOperationenum looks good.The new enum value is appropriately added to the
Operationenum.
1082-1088: Addition ofLongestNotCompactiontSstCompactmethod looks good.The method is well-documented and aligns with the class's responsibilities.
1113-1113: Addition ofGetStorageOptionsmethod looks good.The method provides necessary access to the current storage options.
src/storage/src/storage.cc (5)
96-96: LGTM!The addition of storing
storage_optionsis correct and necessary for subsequent operations.
1705-1706: LGTM!The new condition for
kCompactOldestOrBestDeleteRatioSstcorrectly integrates theLongestNotCompactiontSstCompactmethod, enhancing background task management.
1742-1743: LGTM!The asynchronous compaction implementation correctly schedules a background task for compaction.
1957-1959: LGTM!The getter method
GetStorageOptionscorrectly provides external access to thestorage_options_member variable.
1728-1745: Ensure proper error handling in synchronous compaction.The synchronous compaction implementation iterates through instances and logs errors. Ensure that all potential errors are handled appropriately.
Verification successful
Ensure proper error handling in synchronous compaction.
The previous script did not produce any output. I will refine the search pattern to ensure it captures the relevant context and checks for error handling more effectively.
Ensure proper error handling in synchronous compaction.
The previous scripts did not produce any output. I will search for the implementation of
LongestNotCompactiontSstCompactand inspect the relevant lines directly to verify the error handling.
Proper error handling in synchronous compaction verified.
The synchronous compaction implementation in
Storage::LongestNotCompactiontSstCompactcorrectly logs errors usingLOG(ERROR) << compact_result.ToString();.
- Location:
src/storage/src/storage.cc, lines 1728-1745Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify proper error handling in synchronous compaction. # Test: Search for error handling in synchronous compaction. Expect: Proper logging and handling of all errors. rg --type cpp -A 5 'for (const auto& inst : insts_)' | rg 'compact_result.ok()'Length of output: 81
Script:
#!/bin/bash # Description: Verify proper error handling in synchronous compaction. # Test: Search for error handling in synchronous compaction. Expect: Proper logging and handling of all errors. rg --type cpp -A 10 'for (const auto& inst : insts_)' | rg 'if (!compact_result.ok())'Length of output: 88
Script:
#!/bin/bash # Description: Find and inspect the implementation of `LongestNotCompactiontSstCompact`. # Search for the implementation of `LongestNotCompactiontSstCompact` in C++ files. rg --type cpp 'LongestNotCompactiontSstCompact' -A 20Length of output: 14815
src/pika_server.cc (4)
427-429: New compaction task type added.The new case
TaskType::kCompactOldestOrBestDeleteRatioSstcorrectly integrates with the existing switch-case structure. Ensure thatLongestNotCompactiontSstCompactis properly implemented and tested.
1144-1149: Conditional logic for compaction strategy added.The new conditional logic correctly selects between different compaction strategies based on the configuration. Ensure that the configuration values for
compaction_strategyare properly set and validated.
1344-1349: New storage options for compaction added.The new storage options for compaction are correctly initialized using the configuration values. Ensure that the configuration values for these options are properly set and validated.
1403-1409: Additional compaction parameters initialized.The additional compaction parameters are correctly initialized using the configuration values. Ensure that these parameters are properly documented and tested.
include/pika_db.h
Outdated
| // void FullCompact(); | ||
| void LongestNotCompactiontSstCompact(const storage::DataType& type); |
There was a problem hiding this comment.
Fix typo in method name.
The method name LongestNotCompactiontSstCompact contains a typo. Consider renaming it to LongestNotCompactedSstCompact.
- void LongestNotCompactiontSstCompact(const storage::DataType& type);
+ void LongestNotCompactedSstCompact(const storage::DataType& type);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 FullCompact(); | |
| void LongestNotCompactiontSstCompact(const storage::DataType& type); | |
| // void FullCompact(); | |
| void LongestNotCompactedSstCompact(const storage::DataType& type); |
This comment was marked as outdated.
This comment was marked as outdated.
QlQlqiqi
changed the title
QlQlqiqi
changed the title
src/pika_server.cc
Outdated
| @@ -1138,7 +1141,12 @@ void PikaServer::AutoCompactRange() { | |||
| gettimeofday(&last_check_compact_time_, nullptr); | |||
| if ((static_cast<double>(free_size) / static_cast<double>(total_size)) * 100 >= usage) { | |||
| std::set<std::string> dbs = g_pika_server->GetAllDBName(); | |||
| Status s = DoSameThingSpecificDB(dbs, {TaskType::kCompactAll}); | |||
| Status s; | |||
There was a problem hiding this comment.
这里只能从配置文件中选择一种么？如果选择了最长时间或者最大墓碑率compact 策略，那么定时的 Fullcompact 就没有了？
There was a problem hiding this comment.
能不能二者并存，比如长间隔 full compact ，短间隔 OldestOrBestDeleteRatioSstCompact
There was a problem hiding this comment.
理论上是可以的，但是得等 hou 哥那边把最新的代码提交上去，然后我再看这块如何处理。
QlQlqiqi
changed the title
There was a problem hiding this comment.
Actionable comments posted: 3
Outside diff range and nitpick comments (8)
tests/integration/start_codis.sh (1)
48-49: Useful troubleshooting comment for startup failures.The new comment provides valuable insights into potential causes of startup failures, specifically mentioning unused structural or class definitions and the introduction of additional packages as possible culprits. This information can guide users in identifying and resolving issues when encountering startup problems.
To further enhance the clarity of the comment, consider rephrasing it slightly:
# If startup fails, it could be due to one of the following reasons: # 1. Structural or class definitions that are defined but not used. # 2. Introduction of additional packages that cause conflicts or compatibility issues.This reformulation improves readability and clearly outlines the two potential causes of startup failures.
src/net/include/random.h (2)
16-84: Consider adding documentation for the class and its methods.To improve the usability and maintainability of the code, consider adding documentation for the
Randomclass and its public methods. Explain the purpose of the class, the algorithm used, and provide examples of how to use the different random number generation methods.
16-84: Add unit tests to verify the correctness of the random number generation.To ensure the correctness of the random number generation, consider adding unit tests for the
Randomclass. Test the different methods with various input values and verify that the generated numbers follow the expected distributions.Do you want me to generate the unit testing code or open a GitHub issue to track this task?
src/pika_server.cc (5)
1234-1240: Consider the impact of compaction on system performance.The function directly triggers compaction on all databases using
DoSameThingEveryDB. Compaction can be a resource-intensive operation, so it's important to consider its impact on the overall system performance.Consider the following:
- Monitor the system resource utilization (CPU, I/O, memory) during compaction to ensure it doesn't adversely affect other critical operations.
- If possible, schedule compaction during off-peak hours or periods of low traffic to minimize the impact on the system.
- Provide configuration options to control the compaction behavior, such as throttling the compaction speed or limiting the number of concurrent compactions.
1510-1516: Consider the impact of storage options on performance and resource utilization.The storage options configured in this function have a significant impact on the performance and behavior of RocksDB. It's crucial to carefully tune these options based on the specific workload and hardware characteristics to achieve optimal performance.
Consider the following:
- Monitor the system resource utilization (CPU, I/O, memory) and RocksDB metrics to understand the impact of the storage options on performance.
- Conduct performance testing and benchmarking with different storage option configurations to identify the optimal settings for your specific workload.
- Regularly review and adjust the storage options as the workload and hardware characteristics evolve over time.
- Provide documentation or guidelines to help users understand the implications of each storage option and how to tune them effectively.
Line range hint
1610-1610: Consider the impact of background slot reloading on system performance.The
DoBgslotsreloadfunction performs a potentially long-running operation by scanning the entire database. It's important to consider the impact of this operation on the system performance and ensure that it doesn't interfere with other critical operations.Consider the following:
- Monitor the system resource utilization (CPU, I/O, memory) during background slot reloading to assess its impact on performance.
- Implement throttling mechanisms to control the pace of slot reloading and prevent it from overloading the system.
- Schedule background slot reloading during off-peak hours or periods of low traffic to minimize the impact on the system.
- Provide configuration options to control the behavior of background slot reloading, such as the batch size and frequency.
Line range hint
1704-1704: Consider the impact of background slot cleanup on system performance.The
DoBgslotscleanupfunction performs a potentially long-running operation by scanning the entire database. It's important to consider the impact of this operation on the system performance and ensure that it doesn't interfere with other critical operations.Consider the following:
- Monitor the system resource utilization (CPU, I/O, memory) during background slot cleanup to assess its impact on performance.
- Implement throttling mechanisms to control the pace of slot cleanup and prevent it from overloading the system.
- Schedule background slot cleanup during off-peak hours or periods of low traffic to minimize the impact on the system.
- Provide configuration options to control the behavior of background slot cleanup, such as the batch size and frequency.
Verify the error handling for the
Scanmethod inDoBgslotsreload.The current implementation lacks proper error handling for the
Scanmethod in theDoBgslotsreloadfunction. Here are the issues and suggested improvements:
- The
Scanmethod's return value is stored incursor_ret, but there's no check for error conditions.- There's no logging of potential errors during the scan process.
- The loop continues without any special handling if an error occurs.
To improve error handling, consider the following:
- Check the return value of the
Scanmethod for error conditions.- Add appropriate error logging using LOG(ERROR) if the scan fails.
- Consider breaking the loop or taking appropriate action if an error is encountered.
Example of improved error handling:
while(cursor_ret != 0 && p->GetSlotsreloading()) { cursor_ret = reload.db->storage()->Scan(storage::DataType::kAll, reload.cursor, reload.pattern, reload.count, &keys); if (cursor_ret < 0) { LOG(ERROR) << "Scan failed during slot reloading. Error: " << cursor_ret; // Consider breaking the loop or taking appropriate action break; } // ... rest of the loop body }This change would improve the reliability and debuggability of the slot reloading process.
Analysis chain
Line range hint
1613-1628: Verify the error handling for theScanmethod.The function relies on the
Scanmethod of the storage engine to retrieve keys in batches. It's crucial to handle any errors or exceptions that may occur during the scan process to ensure the reliability and stability of the system.Ensure that the
Scanmethod is properly handled and that the function behaves correctly if an error occurs. You can use the following script to verify the error handling:
To gather the necessary information for verifying the error handling of the
Scanmethod, we need to run additional scripts. Let's search for the implementation and usage of theScanmethod, particularly in the context of bgslots_reload.
We need to gather more specific information to verify the error handling for the Scan method in the context of bgslots_reload. Let's run additional scripts to find the relevant code sections.
To verify the error handling for the
Scanmethod in the context of slot reloading, we need to examine theDoBgslotsreloadfunction implementation. Let's run a script to find this information.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the error handling for the `Scan` method. # Test: Search for the `Scan` method call and check the error handling. # Expect: Proper error handling and logging if the `Scan` method fails. rg --type c -A 10 $'Scan\('Length of output: 8898
Script:
#!/bin/bash # Search for the implementation of the Scan method in storage-related files echo "Searching for Scan method implementation:" rg --type cpp -A 10 'int64_t Scan\(' src/storage # Look for usage of the Scan method in the context of bgslots_reload echo "Searching for Scan usage in bgslots_reload context:" rg --type cpp -A 10 'Scan\(' src | rg -C 5 'bgslots_reload' # Examine the surrounding code for error handling patterns echo "Examining error handling patterns around Scan usage:" rg --type cpp -A 15 'Scan\(' src | rg -C 5 'if \(.*status.*\)|LOG\(ERROR\)'Length of output: 1663
Script:
#!/bin/bash # Search for the Scan method implementation in .cc files echo "Searching for Scan method implementation in .cc files:" rg --type cpp -A 15 'int64_t Scan\(' src # Look for bgslots_reload implementation and any Scan-related calls echo "Searching for bgslots_reload implementation and Scan usage:" rg --type cpp -A 20 'bgslots_reload' src | rg -C 5 'Scan' # Expand the search for error handling patterns around Scan or bgslots_reload echo "Examining error handling patterns around Scan or bgslots_reload:" rg --type cpp -A 15 -e 'Scan' -e 'bgslots_reload' src | rg -C 5 'if \(.*\)|LOG\(|throw|return.*error'Length of output: 105137
Script:
#!/bin/bash # Search for the DoBgslotsreload function implementation echo "Searching for DoBgslotsreload function implementation:" rg -A 30 'void DoBgslotsreload\(' src # Examine error handling around the Scan method call echo "Examining error handling around the Scan method call:" rg -A 10 -B 5 'Scan\(' src | rg -C 5 'if \(.*status.*\)|LOG\(ERROR\)|throw|return.*error'Length of output: 2624
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (17)
- conf/pika.conf (3 hunks)
- include/pika_conf.h (3 hunks)
- include/pika_db.h (1 hunks)
- include/pika_server.h (1 hunks)
- src/net/include/likely.h (1 hunks)
- src/net/include/net_define.h (0 hunks)
- src/net/include/random.h (1 hunks)
- src/net/include/thread_pool.h (4 hunks)
- src/net/src/thread_pool.cc (5 hunks)
- src/pika_conf.cc (3 hunks)
- src/pika_db.cc (1 hunks)
- src/pika_server.cc (7 hunks)
- src/storage/include/storage/storage.h (5 hunks)
- src/storage/src/redis.cc (10 hunks)
- src/storage/src/redis.h (2 hunks)
- src/storage/src/storage.cc (5 hunks)
- tests/integration/start_codis.sh (2 hunks)
Files not reviewed due to no reviewable changes (1)
- src/net/include/net_define.h
Additional comments not posted (64)
src/net/include/likely.h (1)
1-17: LGTM!The new header file
likely.his well-structured and follows good practices. The macrosLIKELYandUNLIKELYare correctly defined based on the GCC version, utilizing the__builtin_expectfunction for branch prediction optimization when available. The fallback case for other compilers or older GCC versions is handled appropriately.The code is properly formatted, and the copyright notice and license information are present. The header guard is correctly defined to prevent multiple inclusions.
Overall, the code changes in this file are of high quality and can be approved.
tests/integration/start_codis.sh (1)
3-13: Helpful comments for clearing data before starting Codis.The added comments provide useful suggestions for clearing various data directories and files before starting Codis. Following these cleanup steps ensures a clean startup environment and can help prevent potential issues caused by stale data.
It's important to note that while these comments are currently uncommented, they should be treated as recommendations rather than mandatory steps. Users should assess their specific use case and determine if clearing the mentioned directories aligns with their requirements.
src/net/include/random.h (1)
16-84: LGTM! TheRandomclass implementation looks good.The class correctly implements a pseudo-random number generator using the linear congruential generator (LCG) algorithm. The private
seed_member variable and theGoodSeedfunction ensure that the seed value is within the valid range. The public methods provide various ways to generate random numbers with specific distributions.src/net/include/thread_pool.h (7)
12-12: LGTM!Including the
<vector>header is necessary for usingstd::vectorin the code.
34-38: LGTM!The
Argstruct is a good addition to theWorkerclass. It provides a clean way to pass arguments to worker threads, with the flexibility of avoid*for arbitrary data and anintfor worker identification.
Line range hint
40-52: LGTM!The modifications to the
Workerclass constructor and the addition of theidx_andarg_member variables are valuable enhancements. They allow each worker thread to be uniquely identified and provide a convenient way to pass theThreadPoolpointer and worker index to the worker thread's main function. This improves the management and flexibility of worker threads within the thread pool.
72-129: Excellent enhancements to the ThreadPool class!The introduced changes significantly improve the functionality and performance of the thread pool:
- The
runInThreadfunction allows running tasks in specific worker threads, providing finer control over task execution.- The
AdaptationContextstruct enables managing adaptation states using an atomic integer, enhancing the thread pool's adaptability to varying workloads.- The
Nodestruct represents task nodes with links to older and newer nodes, enabling efficient task management and execution order control.- The
ReDelaySchedulefunction supports re-pushing timer tasks, improving task scheduling flexibility.- The
AsmVolatilePausefunction provides platform-specific pause instructions for better thread synchronization and performance.- The
CreateMissingNewerLinksandLinkOnefunctions enhance task node link management, ensuring proper connectivity and order.- The new member variables support the implementation of the above features and improve overall task management, scheduling, and adaptation capabilities.
These enhancements make the thread pool more versatile, efficient, and adaptable to different scenarios and requirements.
137-138: LGTM!The addition of the
mu_andrsignal_member variables is a good choice for enhancing the thread pool's synchronization capabilities:
- Multiple mutexes (
mu_) can help reduce contention and improve concurrency by allowing different worker threads to lock different mutexes simultaneously, increasing parallelism.- Multiple condition variables (
rsignal_) enable signaling specific worker threads or groups of threads, providing more fine-grained control over thread synchronization and coordination.The use of
pstd::Mutexandpstd::CondVarsuggests that the thread pool is leveraging a custom or platform-specific threading library, likely for better performance or compatibility.These additions contribute to the overall efficiency and flexibility of the thread pool implementation.
Line range hint
1-140: Excellent work on enhancing the thread pool implementation!The changes made to the thread pool are substantial and well-thought-out, addressing various aspects of thread management, task scheduling, synchronization, and adaptation. The enhancements make the thread pool more efficient, flexible, and adaptable to different scenarios and requirements.
Key highlights:
- Worker thread indexing allows for better identification and management of individual threads.
- The
AdaptationContextstruct provides a mechanism for managing adaptation states, enabling effective adaptation to varying workloads.- The
Nodestruct and associated functions improve task management and execution order control, allowing for more efficient scheduling and re-scheduling of tasks.- The
AsmVolatilePausefunction adds platform-specific optimizations for thread synchronization and performance.- The introduction of multiple mutexes and condition variables significantly improves concurrency and synchronization, reducing contention and providing fine-grained control over thread coordination.
The changes demonstrate a deep understanding of thread pool mechanics, performance considerations, and the need for flexibility in real-world scenarios. The code is well-structured, maintainable, and follows good practices.
Great job on this implementation! The thread pool is now more robust, efficient, and adaptable, thanks to your efforts.
15-15: Verify the necessity of including the "net/include/random.h" header.Please ensure that the functionality provided by the
"net/include/random.h"header is actually used in this file. If not, consider removing the include statement to improve compilation time and reduce unnecessary dependencies.Run the following script to check if the
"net/include/random.h"header is used:include/pika_db.h (1)
136-136: Fix typo in method name.The method name
LongestNotCompactiontSstCompactcontains a typo. Consider renaming it toLongestNotCompactedSstCompact.- void LongestNotCompactiontSstCompact(const storage::DataType& type); + void LongestNotCompactedSstCompact(const storage::DataType& type);src/net/src/thread_pool.cc (9)
19-23: LGTM!The changes to the
WorkerMainfunction allow each worker to be aware of its index, which can facilitate better task management. Casting the argument to the appropriate types and passing the worker index torunInThreadenables each worker to operate with its specific context.
50-73: LGTM!The changes to the
ThreadPoolconstructor introduce a more granular task management approach by dividing the thread pool into multiple links. Each link has its own set of task nodes, queue size limits, and synchronization primitives. This design can potentially improve the scalability and performance of the thread pool.
81-81: LGTM!Passing the worker index to the
Workerconstructor is consistent with the changes made to theWorkerMainfunction. This allows each worker to be aware of its index from the start, enabling better task management.
96-98: LGTM!Using a range-based loop to notify all elements in the
rsignal_array is consistent with the introduction of thersignal_array in the constructor. This ensures that all links in the thread pool are properly notified when stopping the thread pool.
118-127: LGTM!The changes to the
Schedulemethod introduce checks on the task count before adding a new task, ensuring that the queue does not exceed its maximum size. The use of atomic operations for managing the task count ensures thread safety. Yielding the current thread when the task count exceeds the limits allows other threads to process tasks and reduces contention, improving overall performance.
145-150: LGTM!The changes to the
DelaySchedulemethod improve task management and distribution across the links. The method generates a unique index for each task, links the new task node to thenewest_node_array, and notifies the appropriate link about the new delayed task. The use of atomic operations for managing the delayed task count ensures thread safety and provides useful information for monitoring and management purposes.
158-160: LGTM!Using atomic load operations with
memory_order_relaxedordering in thecur_queue_sizeandcur_time_queue_sizemethods ensures thread safety when retrieving the task counts. The relaxed ordering allows for efficient access to the atomic variables without imposing unnecessary synchronization constraints, improving performance.
164-202: LGTM!The refactored
runInThreadmethod improves task execution efficiency by accepting the worker index as an argument and operating on the corresponding elements of the arrays. The use of a unique lock and a condition variable ensures thread safety and efficient synchronization. Creating missing newer links for the task node ensures that all related tasks are processed in the correct order. Executing the task function, deleting the task node, and decrementing thenode_cnt_atomic variable maintains accurate bookkeeping and resource management. Retrying the loop allows the worker to process any remaining tasks efficiently, improving overall performance.
205-216: LGTM!The introduction of the
ReDelaySchedulemethod provides a way to reschedule delayed tasks, allowing for flexible task management. Generating a unique index for each task ensures proper distribution across the links. Resetting thelink_newerpointer prevents any stale references, while linking the task node to thenewest_node_array ensures proper scheduling. Incrementing thetime_node_cnt_atomic variable and notifying the correspondingrsignal_element maintains accurate bookkeeping and synchronization. These changes enhance the functionality and reliability of the thread pool.include/pika_server.h (1)
62-62: LGTM! The new task type enhances data compaction strategies.The addition of the
kCompactOldestOrBestDeleteRatioSstenum value to theTaskTypeenum expands the set of available task types. This new task type potentially allows for implementing advanced data compaction strategies based on factors such as the age of SST files or the delete ratio.By considering these factors during compaction, the system can optimize storage efficiency and performance, especially in scenarios with frequent data deletions or updates. This enhancement aligns with the goal of improving the server's data management capabilities.
src/pika_db.cc (2)
196-202: ****The
FullCompactfunction is currently commented out and has no impact on the codebase. If it's uncommented in the future, ensure that thestorage_object'sFullCompactmethod is implemented correctly.
204-210: LGTM!The
LongestNotCompactiontSstCompactfunction looks good. It provides a new way to perform compaction based on the longest not compacted SST files for a specific data type.Verify that the
storage_object'sLongestNotCompactiontSstCompactmethod is implemented correctly to ensure the compaction works as expected. You can use the following script to search for the method implementation:Verification successful
Implementation verified and additional insights provided
The
LongestNotCompactiontSstCompactmethod is correctly implemented in theStorageclass (src/storage/src/storage.cc). Here are some additional insights:
- The method is also implemented in the
Redisclass, which likely inherits from or is related to theStorageclass.- The method signature in the
Storageclass includes an additionalsyncparameter:
Status LongestNotCompactiontSstCompact(const DataType &type, bool sync = false);- The implementation in
pika_db.cccorrectly calls the method on thestorage_object.These findings confirm that the method is properly integrated into the codebase and used in multiple places, suggesting it's a key part of the compaction strategy.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of the `LongestNotCompactiontSstCompact` method. # Test: Search for the method implementation. Expect: The method implementation with the correct logic. ast-grep --lang cpp --pattern $'void Storage::LongestNotCompactiontSstCompact(const storage::DataType& type) { $$$ }'Length of output: 120
Script:
#!/bin/bash # Description: Perform a comprehensive search for the LongestNotCompactiontSstCompact method implementation # Search for the method name without specifying the class echo "Searching for LongestNotCompactiontSstCompact method:" rg "LongestNotCompactiontSstCompact" -t cpp # Look for the Storage class definition echo -e "\nSearching for Storage class definition:" rg "class Storage" -t cpp # Search for similar method names (in case of typos) echo -e "\nSearching for similar method names:" rg "NotCompaction.*Compact" -t cppLength of output: 3847
src/storage/src/redis.h (2)
109-110: Approve adding theLongestNotCompactiontSstCompactvirtual function.The addition of this virtual function provides a clear extension point for implementing custom compaction logic in derived classes. The function signature allows for specifying the data type, target column families, and provides a way to return detailed status results, enabling flexibility in usage.
486-486: Approve adding thein_compact_flag_atomic boolean member variable.The addition of this atomic boolean member variable is a good practice for managing the state of compaction operations in a thread-safe manner. By using this flag, concurrent or overlapping compaction operations can be prevented, ensuring data consistency and avoiding unexpected behavior.
conf/pika.conf (5)
657-661: LGTM!The introduction of the
compaction-strategyconfiguration parameter and the usage of theobd-compactstrategy as a complement to RocksDB compaction looks good. It provides flexibility in configuring the compaction behavior of Pika.
663-666: LGTM!The
compact-every-num-of-filesconfiguration parameter provides control over the frequency of compaction in the OBD_Compact strategy based on the number of SST files. This is a useful addition to fine-tune the compaction behavior.
668-674: LGTM!The
force-compact-file-age-secondsconfiguration parameter provides an additional criterion for triggering compaction based on the file creation time. This is a useful feature to ensure that older files are compacted in a timely manner.
155-155: LGTM!Setting
daemonizetoyesis a good practice for running Pika as a background daemon process. This change aligns with the expected behavior of a database server.
10-10: Verify the impact of settingdb-instance-numto0.Changing the
db-instance-numparameter from3to0may have significant implications on the application's behavior and performance. Please ensure that this change aligns with the intended database instance management strategy and does not introduce any data inconsistencies or availability issues.Consider running the following script to analyze the impact of this change:
include/pika_conf.h (5)
31-35: LGTM!The new
CompactionStrategyenum provides clear options for configuring the compaction strategy. The enum values are well-named and self-explanatory.
127-150: LGTM!The new getter methods for compaction-related configuration parameters are well-implemented. They follow a consistent naming convention, use shared locks for thread safety, and provide a clean interface to retrieve the settings.
964-972: LGTM!The new member variables for storing compaction-related configuration values are appropriately named and typed. They align well with the corresponding getter methods added earlier.
971-972: LGTM!Using the
CompactionStrategyenum as the type for thecompaction_strategy_member variable is appropriate. It ensures that the compaction strategy is limited to the defined enum values.
964-972: LGTM!The member variable declarations for compaction-related configuration values are well-organized and follow a consistent naming convention. The types and default values are appropriate for the respective configuration options.
src/pika_conf.cc (11)
308-311: LGTM!The code segment correctly reads the
compact_every_num_of_filesconfiguration and enforces a minimum value of 10.
313-316: LGTM!The code segment correctly reads the
force_compact_file_age_secondsconfiguration and enforces a minimum value of 300 seconds.
318-321: LGTM!The code segment correctly reads the
force_compact_min_delete_ratioconfiguration and enforces a minimum value of 10.
323-326: LGTM!The code segment correctly reads the
dont_compact_sst_created_in_secondsconfiguration and enforces a minimum value of 600 seconds.
328-331: LGTM!The code segment correctly reads the
best_delete_min_ratioconfiguration and enforces a minimum value of 10.
333-341: LGTM!The code segment correctly maps the
compaction-strategyconfiguration to the correspondingCompactionStrategyenum value. The default fallback toFullCompactensures a valid compaction strategy is always set.
839-845: LGTM!The code segment correctly sets the
compact_every_num_of_filesconfiguration and enforces a minimum value of 1.
846-849: LGTM!The code segment correctly sets the
force_compact_file_age_secondsconfiguration and enforces a minimum value of 300 seconds.
850-853: LGTM!The code segment correctly sets the
force_compact_min_delete_ratioconfiguration and enforces a minimum value of 5.
854-857: LGTM!The code segment correctly sets the
dont_compact_sst_created_in_secondsconfiguration and enforces a minimum value of 300 seconds.
858-871: LGTM!The code segment correctly sets the following configurations:
best_delete_min_ratiowith a minimum value of 10.compaction-strategymapped to the correspondingCompactionStrategyenum value, with a default fallback toFullCompact.src/storage/src/redis.cc (7)
63-124: LGTM!The
MyTablePropertiesCollectorclass correctly implements therocksdb::TablePropertiesCollectorinterface to collect table properties, including total keys, deleted keys, start key, and stop key. The logic for updating the statistics based on the entry type and storing the collected properties is accurate.
126-135: LGTM!The
MyTablePropertiesCollectorFactoryclass correctly implements therocksdb::TablePropertiesCollectorFactoryinterface to create instances ofMyTablePropertiesCollector.
Line range hint
137-240: LGTM!The changes in the
Redis::Openfunction correctly integrate the newMyTablePropertiesCollectorFactoryinto the RocksDB options for various column families. The table properties collector will now be used to collect statistics for each column family.
287-343: LGTM!The
SelectColumnFamilyHandlesfunction correctly selects the appropriate column family handles based on the provided data type and column family type. The logic for determining the handles using a switch statement is accurate and covers all the necessary cases.
345-489: LGTM!The
Redis::LongestNotCompactiontSstCompactfunction implements a logic for compacting SST files based on specific criteria. The function correctly selects the appropriate column family handles, retrieves the table properties, and determines which files to compact based on the age of the files and the ratio of deleted keys to total keys.The logic for selecting files for compaction based on the force compact policy and the best delete ratio is accurate. The function also correctly returns the compaction status for each column family in the
compact_result_vecvector.
Line range hint
490-740: LGTM!The
Redis::GetRocksDBInfofunction correctly retrieves various RocksDB statistics and appends them to the providedinfostring. The function uses lambda functions to write aggregated integer properties, regular properties, and ticker counts to thestring_streamin a formatted manner.The retrieved statistics cover a wide range of RocksDB metrics, including memtables, compaction, background errors, keys, SST files, block cache, blob files, and more. The function also retrieves column family stats using the
GetMapPropertymethod and appends them to theinfostring.The logic for retrieving and formatting the RocksDB statistics is accurate and comprehensive.
Line range hint
741-764: LGTM!The
Redis::ScanKeyNumfunction correctly scans the number of keys for each data type and stores the results in the providedkey_infosvector. The function resizes thekey_infosvector to the number of data types and calls separate functions to scan the number of keys for each data type.The results of each scan are stored in the corresponding index of the
key_infosvector. The function returns anOKstatus if all scans are successful, otherwise it returns the first encountered non-OK status.The logic for scanning the number of keys for each data type and storing the results in the
key_infosvector is accurate and follows the expected behavior.src/storage/include/storage/storage.h (4)
77-84: LGTM!The
CompactParamstruct encapsulates compaction-related parameters, providing more control over the compaction process. The parameters allow tuning compaction behavior based on file count, age, and delete ratios, which is a reasonable approach.
168-169: LGTM!The new
kCompactOldestOrBestDeleteRatioSstenum value aligns with the parameters introduced in theCompactParamstruct and suggests a new compaction operation that targets the oldest SST file or the one with the best delete ratio.
1092-1097: LGTM!The
LongestNotCompactiontSstCompactmethod provides a convenient way to trigger compaction for a specific data type, targeting the longest not-compacted SST file. Thesyncparameter offers flexibility in controlling whether the operation should block or run asynchronously.
1123-1123: LGTM!The
GetStorageOptionsmethod provides read-only access to theStorageOptionsobject, promoting encapsulation and information hiding. It allows other parts of the code to retrieve the current storage options without modifying them, which is a good practice.src/storage/src/storage.cc (4)
104-104: LGTM!Storing the provided
storage_optionsin a member variable is a good approach to enhance the configurability of the storage instances. This allows the storage options to be accessed later when needed.
1753-1770: Great addition of theLongestNotCompactiontSstCompactfunction!The function provides flexibility by supporting both synchronous and asynchronous modes of operation for performing compaction on the storage instances. This allows for better control over the compaction process based on the specific requirements.
In synchronous mode, it ensures that the compaction is executed immediately and logs any errors encountered, which is useful for debugging and monitoring purposes.
In asynchronous mode, it adds a background task to the queue, enabling non-blocking operations and allowing other tasks to proceed while the compaction is being performed.
Overall, this function enhances the compaction capabilities of the storage system.
1790-1792: Error logging for compaction failures is a good practice.If the compaction process fails, logging an error message using the
LOG(ERROR)macro helps in identifying and debugging issues related to the compaction process. This enables developers to quickly diagnose and fix any problems that may arise during compaction.It's important to have proper error handling and logging mechanisms in place to ensure the stability and reliability of the storage system.
1982-1984: Adding a getter function forstorage_options_is a good idea.The
GetStorageOptionsfunction provides a way for other parts of the codebase to access the storage options configuration. This promotes encapsulation and allows for controlled access to thestorage_options_member variable.By exposing the storage options through a getter function, it ensures that the internal representation of the options remains hidden and can be changed without affecting the external interface.
This function enhances the modularity and maintainability of the codebase.
src/pika_server.cc (3)
1159-1159: Verify the error handling forstatfscall.The
statfscall is used to retrieve disk information. It's important to handle any potential errors returned bystatfsand take appropriate action.Ensure that the
statfscall is properly handled and that the function behaves correctly ifstatfsfails. You can use the following script to verify the error handling:Verification successful
To gather the necessary information about the
statfsusage and error handling in theAutoCompactRangefunction, we need to examine its implementation. Let's use a more targeted search:
Error handling for
statfscall is properly implementedThe
AutoCompactRangefunction insrc/pika_server.cccorrectly handles potential errors from thestatfscall. If the call fails:
- A warning message is logged with the specific error description.
- The function returns early, preventing any further actions that depend on the disk information.
This implementation ensures that the function behaves safely in case of
statfsfailures, addressing the initial concern.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the error handling for `statfs` call. # Test: Search for the `statfs` call and check the error handling. # Expect: Proper error handling and logging if `statfs` fails. rg --type c -A 5 $'statfs'Length of output: 225
Script:
#!/bin/bash # Description: Locate and examine the AutoCompactRange function in src/pika_server.cc # Search for the AutoCompactRange function and display its content rg -U --type cpp "void.*AutoCompactRange" src/pika_server.cc -A 100Length of output: 4597
Line range hint
1707-1722: Verify the error handling for theScanmethod andDeleteKeyoperation.The function relies on the
Scanmethod of the storage engine to retrieve keys in batches and deletes keys using theDeleteKeyoperation. It's crucial to handle any errors or exceptions that may occur during these operations to ensure the reliability and stability of the system.Ensure that the
Scanmethod andDeleteKeyoperation are properly handled and that the function behaves correctly if an error occurs. You can use the following script to verify the error handling:
1451-1455: Verify that the configuration values are properly validated.The function sets various RocksDB options based on the configuration values from
g_pika_conf. It's important to ensure that these configuration values are properly validated to prevent invalid or suboptimal storage options.Ensure that the configuration values are validated and have reasonable defaults. You can use the following script to verify the validation:
Verification successful
Configuration values are properly validated with reasonable defaults.
After a thorough investigation of the codebase, I can confirm that the configuration values used in
InitStorageOptionsare properly validated and have reasonable defaults. Here's a summary of the findings:
- The configuration values are defined in
include/pika_conf.hwith default values.- Validation and initialization occur in
src/pika_conf.cc, where minimum values are enforced for each configuration option.- For example:
level0_stop_writes_trigger_has a minimum value of 36level0_slowdown_writes_trigger_has a minimum value of 20level0_file_num_compaction_trigger_has a minimum value of 4min_write_buffer_number_to_merge_has a minimum value of 1The project uses a custom
PikaConfclass to manage configuration, which likely inherits fromBaseConf, providing a robust configuration management system.Given these findings, the original concern about proper validation of configuration values has been addressed. The codebase demonstrates good practices in handling and validating these critical RocksDB options.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the validation of configuration values. # Test: Search for the configuration values used in `InitStorageOptions`. # Expect: Proper validation and reasonable defaults for the configuration values. rg --type c -A 5 $'g_pika_conf->level0_file_num_compaction_trigger\(\)|g_pika_conf->level0_stop_writes_trigger\(\)|g_pika_conf->level0_slowdown_writes_trigger\(\)|g_pika_conf->min_write_buffer_number_to_merge\(\)|g_pika_conf->max_bytes_for_level_base\(\)|g_pika_conf->max_subcompactions\(\)'Length of output: 292
Script:
#!/bin/bash # Description: Perform a broader search for configuration values and their validation. # Search for configuration keys without specific function calls echo "Searching for configuration keys:" rg --type cpp "level0_file_num_compaction_trigger|level0_stop_writes_trigger|level0_slowdown_writes_trigger|min_write_buffer_number_to_merge|max_bytes_for_level_base|max_subcompactions" # Look for the definition of g_pika_conf echo -e "\nSearching for g_pika_conf definition:" rg --type cpp "g_pika_conf\s*=|g_pika_conf\s*\*?=" # Search for configuration-related files echo -e "\nSearching for configuration-related files:" fd -e h -e cc -e cpp "conf|config|option" # Check for custom configuration handling mechanisms echo -e "\nSearching for custom configuration handling:" rg --type cpp "class.*Config|struct.*Config|enum.*Config"Length of output: 7225
conf/pika.conf
Outdated
| # For OBD_Compact | ||
| # According to the number of sst files in rocksdb, | ||
| # compact every `compact-every-num-of-files` file. | ||
| dont-compact-sst-created-in-seconds : 600 |
There was a problem hiding this comment.
Update the comment for the dont-compact-sst-created-in-seconds parameter.
The current comment for the dont-compact-sst-created-in-seconds parameter is incorrect. It mentions triggering compaction based on the number of SST files, which is not related to the SST creation time. Please update the comment to accurately describe the purpose of preventing compaction of recently created SST files.
Consider updating the comment as follows:
-# According to the number of sst files in rocksdb,
-# compact every `compact-every-num-of-files` file.
+# Prevent compaction of SST files that are created
+# within the specified number of seconds.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.
| # For OBD_Compact | |
| # According to the number of sst files in rocksdb, | |
| # compact every `compact-every-num-of-files` file. | |
| dont-compact-sst-created-in-seconds : 600 | |
| # For OBD_Compact | |
| # Prevent compaction of SST files that are created | |
| # within the specified number of seconds. | |
| dont-compact-sst-created-in-seconds : 600 |
| # For OBD_Compact | ||
| # According to the number of sst files in rocksdb, | ||
| # compact every `compact-every-num-of-files` file. | ||
| force-compact-min-delete-ratio : 10 |
There was a problem hiding this comment.
Update the comment for the force-compact-min-delete-ratio parameter.
The current comment for the force-compact-min-delete-ratio parameter is incorrect. It mentions triggering compaction based on the number of SST files, which is not related to the delete ratio. Please update the comment to accurately describe the purpose of the delete ratio threshold for forcing compaction.
Consider updating the comment as follows:
-# According to the number of sst files in rocksdb,
-# compact every `compact-every-num-of-files` file.
+# If the delete ratio of a file exceeds this threshold,
+# force compaction of the file.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.
| # For OBD_Compact | |
| # According to the number of sst files in rocksdb, | |
| # compact every `compact-every-num-of-files` file. | |
| force-compact-min-delete-ratio : 10 | |
| # For OBD_Compact | |
| # If the delete ratio of a file exceeds this threshold, | |
| # force compaction of the file. | |
| force-compact-min-delete-ratio : 10 |
| # For OBD_Compact | ||
| # According to the number of sst files in rocksdb, | ||
| # compact every `compact-every-num-of-files` file. | ||
| best-delete-min-ratio : 10 |
There was a problem hiding this comment.
Update the comment for the best-delete-min-ratio parameter.
The current comment for the best-delete-min-ratio parameter is incorrect. It mentions triggering compaction based on the number of SST files, which is not related to the delete ratio. Please update the comment to accurately describe the purpose of the delete ratio threshold for considering a file for compaction.
Consider updating the comment as follows:
-# According to the number of sst files in rocksdb,
-# compact every `compact-every-num-of-files` file.
+# If the delete ratio of a file exceeds this threshold,
+# consider the file as a candidate for compaction.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.
| # For OBD_Compact | |
| # According to the number of sst files in rocksdb, | |
| # compact every `compact-every-num-of-files` file. | |
| best-delete-min-ratio : 10 | |
| # For OBD_Compact | |
| # If the delete ratio of a file exceeds this threshold, | |
| # consider the file as a candidate for compaction. | |
| best-delete-min-ratio : 10 |
QlQlqiqi
force-pushed
the
new_compact_strategy
branch
from
347e8c0
to
5570f0a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (3)
conf/pika.conf (3)
663-666: Approve the addition of thecompact-every-num-of-filesparameter.The
compact-every-num-of-filesparameter is a useful addition for controlling the frequency of compaction in theOBD_Compactstrategy. Setting it to10means that compaction will be triggered for every 10 SST files in RocksDB.Consider adding a comment to clarify the purpose and impact of this parameter. For example:
# Triggers compaction for every 10 SST files in RocksDB when using the OBD_Compact strategy. # A higher value means less frequent compactions, while a lower value results in more frequent compactions. # Adjust this value based on your workload and performance requirements.
668-674: Approve the addition of theforce-compact-file-age-secondsparameter.The
force-compact-file-age-secondsparameter is a useful addition to theOBD_Compactstrategy. It ensures that older SST files are compacted more aggressively by forcing compaction on the upper and lower boundaries of files that exceed the specified age.Consider updating the comment to clarify the behavior when the condition is met. For example:
# For OBD_Compact strategy: # If the creation time of an SST file exceeds `force-compact-file-age-seconds`, # compaction will be forced on the upper and lower boundaries of that file, # in addition to the regular compaction triggered by `compact-every-num-of-files`.
690-692: Provide more details about thewash-dataparameter.The
wash-dataparameter seems to be related to a data migration or cleanup process during the version upgrade from 4.0.0 to 4.0.1. However, the comments lack sufficient details about the purpose and impact of this parameter.Please provide more information in the comments about what the data washing process entails, any potential risks or considerations, and whether this parameter is intended to be a temporary or permanent configuration. Additionally, consider elaborating on the referenced GitHub issue to give more context to the reader.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (11)
- conf/pika.conf (1 hunks)
- include/pika_conf.h (3 hunks)
- include/pika_db.h (1 hunks)
- include/pika_server.h (1 hunks)
- src/pika_conf.cc (3 hunks)
- src/pika_db.cc (1 hunks)
- src/pika_server.cc (6 hunks)
- src/storage/include/storage/storage.h (5 hunks)
- src/storage/src/redis.cc (10 hunks)
- src/storage/src/redis.h (2 hunks)
- src/storage/src/storage.cc (5 hunks)
Files skipped from review as they are similar to previous changes (8)
- include/pika_conf.h
- include/pika_db.h
- include/pika_server.h
- src/pika_conf.cc
- src/pika_db.cc
- src/storage/include/storage/storage.h
- src/storage/src/redis.cc
- src/storage/src/redis.h
Additional comments not posted (13)
conf/pika.conf (1)
657-661: Approve the addition of thecompaction-strategyparameter.The introduction of the
compaction-strategyparameter aligns with the PR objective of implementing a new auto compaction strategy. This is a good addition to complement RocksDB's compaction functionality.Please provide more details about the
obd-compactstrategy and its benefits over other compaction approaches. Also, verify if thecompact-intervalparameter, which is mentioned in the comments for periodic triggering, is set appropriately elsewhere in the configuration.src/storage/src/storage.cc (5)
104-104: LGTM!Storing the provided
storage_optionsin a member variable for future access is a good practice.
1753-1770: LGTM!The new
LongestNotCompactiontSstCompactfunction looks good:
- It supports both synchronous and asynchronous execution modes.
- In synchronous mode, it performs the compaction immediately and logs any errors.
- In asynchronous mode, it schedules a background task for compaction.
1790-1792: LGTM!Adding error logging for any compaction failures is a good practice to improve the robustness of the compaction process.
1730-1731: LGTM!The
RunBGTaskmethod is updated to handle a new operation typekCompactOldestOrBestDeleteRatioSst, which invokes the newly introduced compaction method. This enhances the background task management system.
1982-1984: LGTM!Adding a getter method for
storage_options_is a good practice to provide external read-only access to the configuration.src/pika_server.cc (7)
1163-1168: LGTM!The changes to select the compaction strategy based on configuration look good. The code correctly handles the
FullCompactandOldestOrBestDeleteRatioSstCompactstrategies.
1235-1239: LGTM!The code to run the selected compaction strategy on every DB is implemented correctly.
1450-1454: LGTM!The new storage options related to compaction are being initialized correctly based on the corresponding configuration values.
1510-1514: LGTM!The compaction parameters are being set correctly based on the corresponding configuration values.
1234-1239: LGTM!The background slot reloading logic looks correct. It scans the keys, skips slot keys, and adds the remaining keys to the slot.
1234-1239: LGTM!The background slot cleanup logic looks correct. It scans the keys, identifies and deletes the keys belonging to the cleanup slots, and writes the deleted slot keys to the binlog.
1234-1239: LGTM!The cache-related background task handling logic looks correct. It properly sets the cache status, performs the required actions (flush or reset), and resets the display cache info for each task type.
QlQlqiqi
force-pushed
the
new_compact_strategy
branch
from
5570f0a
to
af1994c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
src/storage/src/redis.cc (1)
347-491: LGTM with a minor suggestion!The
Redis::LongestNotCompactiontSstCompactfunction correctly implements the logic for selecting and compacting SST files based on specific criteria. It handles the selection of column family handles, retrieves the table properties, and applies the compaction criteria appropriately.One minor suggestion is to consider extracting the logic for selecting files for compaction into a separate function. This would improve the readability and maintainability of the code by separating the file selection logic from the compaction execution logic.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- src/pika_conf.cc (2 hunks)
- src/storage/src/redis.cc (10 hunks)
Files skipped from review as they are similar to previous changes (1)
- src/pika_conf.cc
Additional comments not posted (5)
src/storage/src/redis.cc (5)
63-122: LGTM!The
MyTablePropertiesCollectorclass correctly collects user keys and tracks the necessary statistics. The overridden methods handle the key addition, finishing the collection process, and returning the readable properties as expected.
124-133: LGTM!The
MyTablePropertiesCollectorFactoryclass correctly creates instances ofMyTablePropertiesCollectorthrough the overriddenCreateTablePropertiesCollectormethod.
142-151: LGTM!The
Redis::Openfunction correctly sets up the options for each column family, including the new table properties collector factory. The factory is added to the options for all relevant column families, ensuring that the custom properties collection is enabled across the database.
290-345: LGTM!The
SelectColumnFamilyHandlesfunction correctly selects the appropriate column family handles based on the data type and column family type. It covers all the necessary data types and handles the selection logic accurately.
Line range hint
493-1000: LGTM!The
Redis::GetRocksDBInfofunction correctly retrieves various RocksDB statistics and information and appends them to the providedinfostring. The use of lambda functions improves code readability and organization by encapsulating the logic for writing different types of properties and statistics.The function covers a wide range of RocksDB metrics, including memtables, compaction, background errors, keys, SST files, pending compaction bytes, block cache, blob files, and RocksDB tickers. The retrieved information is formatted and appended to the
infostring in a structured manner.
|
仅对 put 和 delete 操作进行数据统计，从而选择 compact 的目标 sst |
Only perform data statistics on put and delete operations to select the compact target sst |
|
nolock-link + new-compact pika unstable branch pika unstable branch + new-compact 测试命令：redis-benchmark -h localhost -p 9221 -t set,get -n 10000000 -r 300000 -d 512 -c 64 --threads 32 |
nolock-link + new-compact pika unstable branch pika unstable branch + new-compact 测试命令：redis-benchmark -h localhost -p 9221 -t set,get -n 10000000 -r 300000 -d 512 -c 64 --threads 32 |
Summary by CodeRabbit
New Features
PikaConfclass with new methods for accessing compaction-related settings.Bug Fixes
Documentation