LocalDB: Retry failed GC requests #17386
Conversation
|
🟢 |
lex007in
force-pushed
the
cleanup_tablet_gc_retry
branch
from
6ef1ac7 to
b8cf79f
Compare
|
⚪ DetailsTest history | Ya make output | Test bloat
⚪ Test history | Ya make output | Test bloat | Test bloat
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
|
⚪ DetailsTest history | Ya make output | Test bloat
⚪ DetailsTest history | Ya make output | Test bloat | Test bloat
⚪ Test history | Ya make output | Test bloat | Test bloat | Test bloat
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
lex007in
force-pushed
the
cleanup_tablet_gc_retry
branch
from
b8cf79f to
0a14d96
Compare
|
⚪ DetailsTest history | Ya make output | Test bloat
⚪ Test history | Ya make output | Test bloat | Test bloat
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
|
⚪ DetailsTest history | Ya make output | Test bloat
⚪ DetailsTest history | Ya make output | Test bloat | Test bloat
⚪ Test history | Ya make output | Test bloat | Test bloat | Test bloat
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
lex007in
force-pushed
the
cleanup_tablet_gc_retry
branch
from
0a14d96 to
4e33302
Compare
lex007in
force-pushed
the
cleanup_tablet_gc_retry
branch
from
4e33302 to
ff2fad2
Compare
|
⚪ DetailsTest history | Ya make output | Test bloat
⚪ Test history | Ya make output | Test bloat | Test bloat
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
|
⚪ DetailsTest history | Ya make output | Test bloat
⚪ DetailsTest history | Ya make output | Test bloat | Test bloat
⚪ Test history | Ya make output | Test bloat | Test bloat | Test bloat
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
| GcLogic->OnCollectGarbageResult(ev); | ||
| DataCleanupLogic->OnCollectedGarbage(OwnerCtx()); | ||
|
|
||
| const bool needRetryFailed = DataCleanupLogic->NeedGC(); |
There was a problem hiding this comment.
Смущает:
- полезно ретраить в принципе всегда, почему только если есть GC в data cleanup logic?
- этот код про ретраи почему-то выполняется всегда, даже если в ответе нет никаких ошибок, и неочевидно, что на самом деле решения о ретраях принимает вызов
TryScheduleGcRequestRetries
Возможно стоит решение о ретраях принимать в вызове GcLogic->OnCollectGarbageResult, и собственно возвращать в результате на сколько шедулить этот ретрай, если нужно. Тогда вся логика будет внутри gc logic, и не будет такого, что часть этой логики почему-то в таблетке снаружи.
There was a problem hiding this comment.
Тут хотел между OnCollectGarbageResult и TryScheduleGcRequestRetries ещё спрашивать у DataCleanupLogic "а точно ли ещё нужнен GC". Но если ретраить всегда, то можно упростить логику. Переделаю, чтоб всегда ретраить.
| if (GcWaitFor == 0) { // all channel's GC completed | ||
| if (CommitedGcBarrier == KnownGcBarrier && TryCounter > 0) { // all GC requests succeeded and we must reset try counter | ||
| TryCounter = 0; | ||
| BackoffTimer.Reset(); |
There was a problem hiding this comment.
Какая-то сложная для понимая логика со сравнением барьеров и т.д. Я бы сделал более явно:
- В
OnCollectGarbageFailureинкрементил счётчик ошибок в текущем запросе - В
OnCollectGarbageSuccessтам жеCollectSent.Clear()делать вот это обнуление, если не было ошибок (а их там и не могло быть, т.к. в случае ошибок делаетсяCollectSent.Clear()и мы туда не дойдём) - В
SendCollectGarbageзанулять счётчик ошибок
Тогда в этом методе можно будет просто проверять счётчик ошибок и если они были - то шедулить запрос.
There was a problem hiding this comment.
Да, зануление можно действительно перенести в OnCollectGarbageSuccess.
Отдельный счётчик ошибок не стал делать, так как посчитал, что текущей информации достаточно. Но можно сделать и явно, да.
| } | ||
|
|
||
| void TExecutorGCLogic::TChannelInfo::RetryGcRequests(const TTabletStorageInfo *tabletStorageInfo, ui32 channel, ui32 generation, const TActorContext& ctx) { | ||
| RetryIsScheduled = false; |
There was a problem hiding this comment.
Мне кажется здесь нужно проверять, что этот флаг установлен (и ничего не делать, если не установлен). Плюс сбрасывать этот флаг, если например SendCollectGarbage вызвался до срабатывания таймера (мы уже новый запрос отправили, если нужно, и ретраить после этого всё-равно нужно новый запрос, а не старый).
There was a problem hiding this comment.
Ну тут логика, что при очередном SendCollectGarbage и ошибке происходит не перепланирование ретрая (как ты по сути предлагаешь), а просто повторные ретраи игнорируются, пока не обработался первый ретрай. В этом варианте создаётся меньше событий EvRetryGcRequest (так как, условно, более старое событие переиспользуется для более новых ошибок, которые возникли к моменту обработки), но ретраи могут происходят чаще (так как не зависят от конкуретных нормальных GC)
|
⚪ DetailsTest history | Ya make output | Test bloat
⚪ DetailsTest history | Ya make output | Test bloat | Test bloat
⚪ Test history | Ya make output | Test bloat | Test bloat | Test bloat
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
|
⚪ DetailsTest history | Ya make output | Test bloat
⚪ Test history | Ya make output | Test bloat | Test bloat
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
lex007in
force-pushed
the
cleanup_tablet_gc_retry
branch
from
983e2ec to
745cc6f
Compare
|
⚪ DetailsTest history | Ya make output | Test bloat
⚪ Test history | Ya make output | Test bloat | Test bloat
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
|
⚪ DetailsTest history | Ya make output | Test bloat
⚪ DetailsTest history | Ya make output | Test bloat | Test bloat
⚪ Test history | Ya make output | Test bloat | Test bloat | Test bloat
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
lex007in
changed the title
2 participants
Changelog entry
...
Changelog category
Description for reviewers
...