fix: v4* compact loop error #3128
Conversation
WalkthroughA new configuration parameter Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant PikaServer
participant PikaConf
participant Storage
User->>PikaServer: CONFIG SET obd-compact-interval <value>
PikaServer->>PikaConf: SetObdCompactInterval(value)
PikaConf-->>PikaServer: Update obd_compact_interval_
loop Every AutoCompactRange interval
PikaServer->>PikaConf: Get obd_compact_interval_
PikaServer->>PikaServer: Check last_strategy_compact_time_
alt If interval elapsed
PikaServer->>Storage: Schedule compaction (if strategy matches)
PikaServer->>PikaServer: Update last_strategy_compact_time_
end
end
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
🧹 Nitpick comments (2)
include/pika_server.h (1)
553-553: Consider using std::chrono for timestamp consistency.Based on the project's established patterns, consider using
std::chrono::steady_clock::time_pointinstead ofstruct timevalfor better type safety and consistency with the codebase's timestamp handling approach.- struct timeval last_strategy_compact_time_; + std::chrono::steady_clock::time_point last_strategy_compact_time_;src/pika_admin.cc (1)
2102-2109: Consider adding upper bound validation for interval parameter.The current validation only checks for non-negative values, but interval parameters typically benefit from reasonable upper bounds to prevent configuration errors.
} else if (set_item == "obd-compact-interval") { if (pstd::string2int(value.data(), value.size(), &ival) == 0 || ival < 0) { res_.AppendStringRaw("-ERR Invalid argument \'" + value + "\' for CONFIG SET 'obd-compact-interval'\r\n"); return; } + // Consider adding reasonable upper bound validation + // if (ival > MAX_REASONABLE_INTERVAL) { + // res_.AppendStringRaw("-ERR Invalid argument \'" + value + "\' for CONFIG SET 'obd-compact-interval', value too large\r\n"); + // return; + // } g_pika_conf->SetObdCompactInterval(ival); res_.AppendStringRaw("+OK\r\n");Otherwise, the implementation correctly:
- Validates integer parsing
- Ensures non-negative values
- Calls the appropriate configuration setter
- Returns proper success/error responses
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
conf/pika.conf(1 hunks)include/pika_conf.h(3 hunks)include/pika_server.h(1 hunks)src/pika_admin.cc(3 hunks)src/pika_conf.cc(2 hunks)src/pika_server.cc(3 hunks)src/storage/include/storage/storage.h(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
include/pika_server.h (2)
Learnt from: hahahashen
PR: OpenAtomFoundation/pikiwidb#338
File: src/client.h:33-48
Timestamp: 2024-06-14T14:09:58.236Z
Learning: The `PClient` class in `src/client.h` uses `std::chrono` for handling timestamps for better type safety and clarity.
Learnt from: hahahashen
PR: OpenAtomFoundation/pikiwidb#338
File: src/client.h:33-48
Timestamp: 2024-10-09T12:55:29.966Z
Learning: The `PClient` class in `src/client.h` uses `std::chrono` for handling timestamps for better type safety and clarity.
src/pika_server.cc (2)
Learnt from: hahahashen
PR: OpenAtomFoundation/pikiwidb#338
File: src/client.h:33-48
Timestamp: 2024-10-09T12:55:29.966Z
Learning: The `PClient` class in `src/client.h` uses `std::chrono` for handling timestamps for better type safety and clarity.
Learnt from: hahahashen
PR: OpenAtomFoundation/pikiwidb#338
File: src/client.h:33-48
Timestamp: 2024-06-14T14:09:58.236Z
Learning: The `PClient` class in `src/client.h` uses `std::chrono` for handling timestamps for better type safety and clarity.
🧬 Code Graph Analysis (2)
src/pika_admin.cc (2)
src/pstd/src/pstd_string.cc (8)
stringmatch(186-189)stringmatch(186-186)string2int(350-439)string2int(350-350)string2int(444-457)string2int(444-444)string2int(462-475)string2int(462-462)src/pstd/include/pstd_string.h (4)
stringmatch(45-45)string2int(48-48)string2int(49-49)string2int(50-50)
src/pika_conf.cc (1)
src/pstd/src/base_conf.cc (4)
GetConfInt(98-109)GetConfInt(98-98)SetConfInt(246-257)SetConfInt(246-246)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Analyze (go)
- GitHub Check: build_on_macos
- GitHub Check: build_on_ubuntu
- GitHub Check: build_on_centos
🔇 Additional comments (11)
src/storage/include/storage/storage.h (1)
84-84: LGTM! Well-integrated storage parameter.The addition of
obd_compact_interval_to theCompactParamstruct is well-placed and follows the existing naming conventions. The integration with the storage options structure is appropriate for the OBD compaction strategy timing control.src/pika_conf.cc (2)
338-341: LGTM! Proper configuration loading with validation.The configuration loading logic correctly uses the standard
GetConfIntmethod and appropriately enforces a minimum value of 600 seconds to prevent overly frequent compaction operations.
890-893: LGTM! Consistent configuration rewriting pattern.The configuration rewriting logic follows the established pattern and correctly enforces the minimum value constraint. While there's minor code duplication with the loading logic, this is consistent with how other configuration parameters are handled in this codebase.
include/pika_conf.h (2)
146-149: LGTM: Getter method follows established pattern.The getter method implementation is correct and consistent with other configuration getters in the class.
1050-1050: LGTM: Member variable declaration is correct.The member variable declaration follows the naming convention and is appropriately placed with other compact-related variables.
conf/pika.conf (1)
684-705: Parameter updates look reasonable for fixing compact loop issues.The changes to the OBD_Compact configuration parameters appear well-thought-out:
- force-compact-file-age-seconds: Increased from 300 to 1200 seconds (5 to 20 minutes) - gives files more time to mature before forced compaction
- force-compact-min-delete-ratio: Increased from 10 to 50 - higher threshold before triggering compaction
- dont-compact-sst-created-in-seconds: Increased from 20 to 300 seconds - protects newer files from unnecessary compaction
- best-delete-min-ratio: Increased from 10 to 30 - optimizes compaction trigger threshold
- obd-compact-interval: New parameter with 600 second minimum - adds periodic control
These adjustments should help reduce the frequency of compaction triggers and address the compact loop error mentioned in the PR objectives. The documentation is also clear and helpful.
src/pika_server.cc (3)
49-49: LGTM: Member variable initialization is correct.The initialization of
last_strategy_compact_time_to{0, 0}is appropriate for atimevalstructure.
1513-1513: LGTM: Storage options configuration is correct.The assignment properly propagates the
obd_compact_intervalconfiguration value to the storage options structure.
1234-1247: No thread-safety issue with last_strategy_compact_time_The
AutoCompactRange()routine (and itsgettimeofday/last_strategy_compact_time_logic) is only ever invoked fromDoTimingTask(), which runs on a single dedicated timing thread—there are no concurrent calls toAutoCompactRange(). Therefore:
- No mutex is needed to guard
last_strategy_compact_time_.- Adding error-checks around
gettimeofday()is a matter of defensive style; in practice it never fails unless passed an invalid pointer.- Switching to
std::chronowould be a global stylistic change rather than a PR-specific fix.Likely an incorrect or invalid review comment.
src/pika_admin.cc (2)
2311-2316: LGTM: Configuration retrieval implementation is correct.The implementation follows the established pattern for configuration parameter retrieval. The pattern matching, element counting, and encoding logic are consistent with other parameters in the codebase.
2379-2379: LGTM: Parameter correctly added to configurable list.The parameter is properly added to the list of configurable parameters returned by
CONFIG SET *.
| void SetObdCompactInterval(const int value) { | ||
| std::unique_lock l(rwlock_); | ||
| obd_compact_interval_ = value < 600 ? 600 : value; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add TryPushDiffCommands call for configuration change tracking.
The setter method correctly enforces the minimum value of 600 seconds and uses proper thread safety with unique_lock. However, it's missing the TryPushDiffCommands call that most other setters use to track configuration changes for persistence.
void SetObdCompactInterval(const int value) {
std::unique_lock l(rwlock_);
+ TryPushDiffCommands("obd-compact-interval", std::to_string(value));
obd_compact_interval_ = value < 600 ? 600 : value;
}📝 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 SetObdCompactInterval(const int value) { | |
| std::unique_lock l(rwlock_); | |
| obd_compact_interval_ = value < 600 ? 600 : value; | |
| } | |
| void SetObdCompactInterval(const int value) { | |
| std::unique_lock l(rwlock_); | |
| TryPushDiffCommands("obd-compact-interval", std::to_string(value)); | |
| obd_compact_interval_ = value < 600 ? 600 : value; | |
| } |
🤖 Prompt for AI Agents
In include/pika_conf.h around lines 991 to 994, the SetObdCompactInterval setter
correctly enforces a minimum value and uses thread safety but lacks the
TryPushDiffCommands call. Add a call to TryPushDiffCommands after setting
obd_compact_interval_ to ensure configuration changes are tracked for
persistence, following the pattern used in other setter methods.
|
@Mixficsol @wangshao1 |
2 participants
fix: #3123
1.修复当前obd-compact bug容易触发、大数据量时full-compact长期执行的问题
2.移除了full-compact定期执行
3.支持obd-compact周期动态修改配置
（未修复可能潜在的obd-compact逻辑bug）
Summary by CodeRabbit
New Features
Improvements
Bug Fixes