Add NewlyZeroredBlocks parameter, add GetTopByGarbageIgnoringZeroed to compaction map#5865
Add NewlyZeroredBlocks parameter, add GetTopByGarbageIgnoringZeroed to compaction map#5865
Conversation
gy2411
added
large-tests
…oes to compaction map
gy2411
force-pushed
the
users/gayurgin/add_garbage_ignoring_zeroes_counters
branch
from
af5a987 to
6754daf
Compare
![robot-vibe-db[bot]](https://avatars.githubusercontent.com/in/3319454?s=60&v=4){kind=small}
There was a problem hiding this comment.
AI Review Summary
Verdict: ✅ No critical issues found
Critical issues
No critical issues found.
Other findings
- Major | Medium:
NewlyZeroedBlocksis not persisted to the database — after tablet restart,GarbageIgnoringZeroedwill equalGarbageBlockCount, losing the protection against compaction bursts —cloud/blockstore/libs/storage/partition/part_actor_addblobs.cpp:524 - Minor | Medium:
UsedBlocksIgnoringZeroed()is defined but never called anywhere — dead code —cloud/blockstore/libs/storage/core/compaction_policy.h:86 - Minor | Medium:
UsedBlocksIgnoringZeroed()has a potentialui16overflow —UsedBlockCount + NewlyZeroedBlockscan wrap around —cloud/blockstore/libs/storage/core/compaction_policy.h:88 - Minor | Low:
TRangeStatparameterized constructor does not includeNewlyZeroedBlocks— callers must rely on the default zero-init, easy to miss when extending —cloud/blockstore/libs/storage/core/compaction_policy.h:45
This review was generated automatically. Critical issues require attention; other findings are advisory.
If this comment was useful, please give it a 👍 — it helps us improve the review bot.
| kv.second.Stat.BlockCount + kv.second.BlocksSkippedByCompaction | ||
| ); | ||
| kv.second.Stat.BlockCount + | ||
| kv.second.BlocksSkippedByCompaction); |
There was a problem hiding this comment.
Severity: Major
Confidence: Medium
NewlyZeroedBlocks is computed and passed to CompactionMap.Update() but is not persisted by db.WriteCompactionMap(), which only writes blobCount and blockCount. After a tablet restart, ReadCompactionMap() constructs TRangeStat with NewlyZeroedBlocks = 0 for every range.
This means GarbageIgnoringZeroed() will equal GarbageBlockCount() after restart, so the protection against compaction bursts following large discards is lost until new writes re-accumulate the tracking data. If a large discard happened right before a restart, the very burst this feature is designed to prevent could occur on startup.
If this is intentional for the "first step" (as described in the PR), consider adding a comment here explaining the transient nature of NewlyZeroedBlocks and tracking the persistence work as a follow-up.
| ui16 UsedBlocksIgnoringZeroed() const | ||
| { | ||
| return UsedBlockCount + NewlyZeroedBlocks; | ||
| } |
There was a problem hiding this comment.
Severity: Minor
Confidence: Medium
Two issues with UsedBlocksIgnoringZeroed():
-
Dead code: This method is defined but never called anywhere in the codebase. If it is intended for a future PR, consider deferring its introduction to when it is actually used.
-
Potential ui16 overflow:
UsedBlockCount + NewlyZeroedBlockscan exceedui16max (65535) if both values are large, causing a silent wrap-around. WhileMaxBlocksCountis 1024, the values are clamped atui16max byUpdateCompactionCounter, so an overflow is theoretically possible.
Suggested fix for overflow (if the method is kept):
ui16 UsedBlocksIgnoringZeroed() const
{
return static_cast<ui16>(Min(
static_cast<ui32>(UsedBlockCount) + NewlyZeroedBlocks,
static_cast<ui32>(Max<ui16>())));
}| ui16 ReadRequestCount = 0; | ||
| ui16 ReadRequestBlobCount = 0; | ||
| ui16 ReadRequestBlockCount = 0; | ||
| ui16 NewlyZeroedBlocks = 0; |
There was a problem hiding this comment.
Severity: Minor
Confidence: Low
The NewlyZeroedBlocks field is added here with a default value of 0, but the parameterized constructor (lines 45-65) does not accept or initialize it. This is consistent with how ReadCompactionMap constructs TRangeStat (since NewlyZeroedBlocks is not persisted), but it creates a subtle inconsistency: a TRangeStat built via the parameterized constructor will always have NewlyZeroedBlocks = 0 regardless of intent.
Consider adding a comment near the constructor noting that NewlyZeroedBlocks is intentionally omitted because it is transient/in-memory only.
2 participants
Notes
We want to add new compaction type to avoid burst of compaction after discards of large ranges: #5815. This is the first step for it.
In this pr:
See details in the issue description.
Issue
#5815