issue-5590: split flushed blobs by compaction range borders#5884
issue-5590: split flushed blobs by compaction range borders#5884vladstepanyuk wants to merge 6 commits intomainfrom
Conversation
vladstepanyuk
added
large-tests
![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
- Minor | High:
Y_DEBUG_ABORT_UNLESSassertion is too strict — will fire for data blobs whenDiskPrefixLengthWithBlockChecksumsInBlobsis 0 (default), sincechecksumsis empty butdataRefsis non-empty —part_actor_flush.cpp:621 - Nit | High:
const ui64 compactionRangeCountinAppendDataBlobstores aui8return value and implicitly narrows it back toui8when passed toTBlobconstructor —part_actor_flush.cpp:575 - Nit | Low:
CountCompactionRangeCountWithSplitOptimizationcounts distinct contiguous range runs whileCountCompactionRangeCountcounts the range index span — names don't convey this difference clearly —part_actor_flush.cpp:538
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.
| FlushBlob({}, std::move(blocks), {}); | ||
| } | ||
|
|
||
| void FlushBlob( |
There was a problem hiding this comment.
IMO it will be better to extract the declaration of this func into a header and write unittests directly for this func
better both from the test coverage point of view and from the test runtime point of view (tablet tests are much heavier than what's actually required to test this func)
There was a problem hiding this comment.
moved flush blocks visitor to partition/model
| const TVector<TBlock>& blocks) const | ||
| { | ||
| ui32 ranges = 1; | ||
| for (size_t i = 1; i < blocks.size(); ++i) { |
There was a problem hiding this comment.
Также, я бы сюда передал SplitByCompactionRangeMaxBlobCount и ограничил цикл по нему. Для рандомной нагрузки по 4k тут же сотни диапазонов могут быть?
There was a problem hiding this comment.
ну тут больше 1024 блоков и 32 диапазона не может быть, для рандомной нагрузки 4к в нынешней картине это итерация по двум элементам
vladstepanyuk
force-pushed
the
users/vladstepanyuk/issue-5590/split-blobs-by-compaction-range-borders/0
branch
from
eb54401 to
1924294
Compare
| } | ||
|
|
||
| template <typename TTmpContainerType> | ||
| void TFlushBlocksVisitor::FlushBlobImpl( |
There was a problem hiding this comment.
А FlushBlobImpl точно нужен шаблонный? Решение о типе внутри нельзя принять и передать уже в шаблон SplitBlocksByCompactionRangeBorders?
There was a problem hiding this comment.
Не принципиально особо, просто странно смотрится что мы туда передаём шаблонный тип, но он не используется ни в аргументах этой фуникци, ни в return type
There was a problem hiding this comment.
нам же надо объявить переменную либо с одним типом либом с другим, поэтому если внутри функции это решать, то надо будет весь код после объявления перемнной запихать под if constexpr
ну или с макросами лямбдами развлекаться
3 participants
Notes
When flushing fresh blocks into mixed blobs, if the number of distinct compaction ranges in a candidate blob does not exceed
SplitByCompactionRangeMaxBlobCountvalue, the blob may be split at compaction range borders to eliminate reading block masks for such blobs. Zero disables splitting.Issue
#5590