Clean log service log path after bootstrap#265
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughAdds filesystem cleanup during DataSubstrate shutdown when bootstrap mode is active, reorders includes in that source file, introduces a project-wide .editorconfig, and updates the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
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. Comment |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
core/src/data_substrate.cpp (2)
252-279: Consider adding path validation for safety.While bootstrap mode is a controlled scenario, adding validation to ensure paths are within the expected data directory would provide an extra safety layer against misconfiguration. For example, verify that paths start with
core_config_.data_path.However, if bootstrap cleanup is always expected to be safe and paths are strictly controlled, the current implementation may be acceptable.
Would you like me to generate a safer implementation with path validation?
26-27: Remove the unused<iostream>include.The
<filesystem>include is necessary forstd::filesystem::remove_all, but<iostream>is not used anywhere in this file. Removing it reduces compilation overhead.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
core/src/data_substrate.cpp(2 hunks)
🔇 Additional comments (4)
core/src/data_substrate.cpp (4)
31-31: LGTM - Include reordering.The include order change appears intentional and doesn't affect functionality.
247-251: LGTM - Safe path capture before resource destruction.Capturing the storage paths before resetting
log_server_tonullptrensures the paths remain accessible after the server is destroyed. This is the correct approach.
269-271: LGTM - Safe substring operation.The
substr(8)call is safe because line 269 verifies thatstorage_pathstarts with"local://"(which is exactly 8 characters), ensuring the string is long enough.
252-279: The inconsistency is intentional and correct—no fix needed.The different prefix handling between RocksDB and log storage paths reflects their different initialization patterns:
- RocksDB storage path is constructed in
log_init.cppline 279 by stripping the"local://"prefix:log_path.substr(8) + "/rocksdb". This path is stored without the prefix and returned as-is byGetRocksDBStoragePath().- Log storage path is passed to the log server with the
"local://"prefix intact and returned byGetStoragePath()with that prefix still present, so it requires stripping at cleanup time.The code correctly reflects that RocksDB paths never carry the
"local://"prefix, while log paths do.
aecb888 to
c42694d
Compare
Here are some reminders before you submit the pull request
fixes eloqdb/tx_service#issue_id./mtr --suite=mono_main,mono_multi,mono_basicSummary by CodeRabbit
Bug Fixes
Style
Chores
✏️ Tip: You can customize this high-level summary in your review settings.