Revert "storage: manage priority logs in housekeeping_loop"#29439
Conversation
There was a problem hiding this comment.
Pull request overview
This PR reverts a previous change that introduced priority partition management in the housekeeping loop. The revert was necessary due to undefined behavior discovered during code review.
Changes:
- Removed priority partition tracking and compaction logic
- Simplified housekeeping loop to remove preemption mechanism
- Removed configuration option for priority compaction timeout
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/v/storage/log_manager.h | Removed priority partition helper methods, preemption logic, and priority logs list |
| src/v/storage/log_manager.cc | Removed priority compaction implementation, preemption timer logic, and restored original housekeeping flow |
| src/v/storage/BUILD | Removed abort_source dependency |
| src/v/config/configuration.h | Removed log_compaction_max_priority_wait_ms property |
| src/v/config/configuration.cc | Removed log_compaction_max_priority_wait_ms configuration initialization |
| , segment_size_jitter(0) // For deterministic behavior in unit tests. | ||
| , compacted_segment_size(config::mock_binding<size_t>(256_MiB)) | ||
| , max_compacted_segment_size(config::mock_binding<size_t>(5_GiB)) | ||
|
|
There was a problem hiding this comment.
Remove this blank line. The line was added in the revert but appears to be unintentional as it adds unnecessary whitespace after the max_compacted_segment_size initialization.
| current_log.flags |= bflags::compacted; | ||
| current_log.last_compaction = ss::lowres_clock::now(); | ||
|
|
There was a problem hiding this comment.
The compaction flags and timestamp are being set before housekeeping actually runs, which is incorrect. In the original implementation being restored, these were set after housekeeping completed successfully. Move these assignments (lines 385-386) to after the housekeeping call at line 479 to properly reflect when compaction actually completes.
| current_log.flags |= bflags::compacted; | |
| current_log.last_compaction = ss::lowres_clock::now(); |
Reverts #29382
#29435 (comment)
^ Undefined behavior above, I'll look at re-adding this behavior when I have time to diagnose the bug.
Backports Required
Release Notes