DataShard and SchemeShard: handle borrowed parts in data erasure #15451
Conversation
|
🟢 |
|
⚪ Test history | Ya make output | Test bloat
⚪ Test 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 |
|
⚪ Test 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
changed the title
|
⚪ |
|
⚪ |
lex007in
force-pushed
the
borrow
branch
2 times, most recently
from
3414a3e to
3d62e99
Compare
|
⚪ Test history | Ya make output | Test bloat
⚪ Test 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 |
|
⚪ Test history | Ya make output | Test bloat
⚪ Test 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 |
|
⚪
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
|
⚪
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
| Response = std::make_unique<TEvDataShard::TEvForceDataCleanupResult>( | ||
| record.GetDataCleanupGeneration(), | ||
| Self->TabletID(), | ||
| NKikimrTxDataShard::TEvForceDataCleanupResult::FAILED); |
There was a problem hiding this comment.
Минорное: хорошо кроме статуса без каких-либо подробностей отправлять ещё какой-то ErrorReason, который бы можно было залоггировать на стороне клиента (например SchemeShard'а). Также меня несколько смущает, что эта ошибка будет повторяться, пока на шарде что-то не изменится, но о том что что-то изменилось узнать нельзя. Будет ли SchemeShard повторять запрос снова и снова?
There was a problem hiding this comment.
Если добавить стоку ErrorReason, то увеличится размер сообщения на ровном месте, хотя может это и не страшно.
Да, SchemeShard будет всё время ретраить, и все ошибки которые может вернуть DataCleanup, можно и нужно ретраить. Можно переименовать как-то явно FAILED -> RETRYABLE_ERROR.
There was a problem hiding this comment.
Сделал больше разных енумов и залогировал.
| LOG_DEBUG_S(ctx, NKikimrServices::FLAT_TX_SCHEMESHARD, | ||
| "TTxCompleteDataErasureShard: data erasure failed at DataShard #" << record.GetTabletId() | ||
| << ", schemestard: " << Self->TabletID()); | ||
| return; // will be retried after timout in the queue |
There was a problem hiding this comment.
Из комментария не ясно что это за таймаут и когда запрос повторят? Точно ли не нужно с ним в очереди что-то сделать?
There was a problem hiding this comment.
Эта очередь так устроена, что нужно явно звать OnDone() для задач, обработка которых завершена (там внутри OnDone задача удаляется из очереди в этот момент). Если не OnDone не вызывать, то потом вызывается обработчик таймаута, который в случае очиски вот тут: https://github.com/ydb-platform/ydb/blob/main/ydb/core/tx/schemeshard/schemeshard__tenant_data_erasure_manager.cpp#L182 -- именно он ретраит в конце. Сам таймаут задаётся в конфиге очистки.
|
⚪ Test history | Ya make output | Test bloat
⚪ Test 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 |
|
⚪ Test 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 |
|
⚪ Test history | Ya make output | Test bloat
⚪ Test 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 |
|
⚪ Test 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 |
|
⚪ Test history | Ya make output | Test bloat
⚪ Test 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 |
| } | ||
| } | ||
| if (DataErasureManager->GetStatus() == EDataErasureStatus::IN_PROGRESS) { | ||
| Execute(CreateTxAddEntryToDataErasure(dataErasureShards), this->ActorContext()); |
There was a problem hiding this comment.
А насколько это вообще безопасно тут делать? Ведь SetPartitioning вызывается из операции split/merge в транзакции, а здесь шедулится какая-то другая транзакция. И завершение split/merge может успешно закоммититься, а до этой транзакции даже очередь не дойдёт. В итоге шарды окажутся просто потерянными? Ну и ещё смущает, что SetPartitioning вызывается в рамках загрузки schemeshard'а, вообще тут кажется никогда не предполагалось какой-то такой сложной логики/действий.
There was a problem hiding this comment.
Да, главное, что смущает -- что SetPartitioning() перегружается несвойственными ей делами. Я уже предлагал посмотреть, как можно переделать.
От вызова SetPartitioning() во время TxInit защищаться и не надо -- это ключевая вещь для продолжения процесса data erasure, если он уже работал до рестарта. Хотя конечно не хватает комментариев c описанием зависимостей в порядке загрузки состояния DataErasureManager и шардов таблиц.
до этой транзакции даже очередь не дойдёт
Это если schemeshard перезапустится?
Тогда статус data erasure будет IN_PROGRESS и как раз SetPartitioning() во время TxInit отработают и обновят актуальный список шардов для чистки. Логически верно, но кажется напряжно. Было бы лучше на рестарте выполнять один проход по общему списку шардов вместо отдельной транзакции на каждую таблицу.
|
⚪ Test 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 |
| if (Self->DataErasureManager->GetStatus() == EDataErasureStatus::IN_PROGRESS) { | ||
| Self->Execute(Self->CreateTxCancelDataErasureShards({ShardIdx})); | ||
| } |
There was a problem hiding this comment.
Почему потребовалось переносить запуск CancelDataErasureShards сюда?
There was a problem hiding this comment.
В прошлом месте таблетка ещё не была удалена, и очистка могла успешно завершится до удаления этой таблетки, что плохо и нарушает гарантии удаления. Сюда же попадаем уже после того, как хайв ответил, что таблетка удалена.
(Да, ещё остаётся проблема, что хайв на самом деле отвечает до того, как удалил данные в блобсторадже, но это собираемся отдельно доделывать).
| } | ||
| } | ||
| if (DataErasureManager->GetStatus() == EDataErasureStatus::IN_PROGRESS) { | ||
| Execute(CreateTxAddEntryToDataErasure(dataErasureShards), this->ActorContext()); |
There was a problem hiding this comment.
Да, главное, что смущает -- что SetPartitioning() перегружается несвойственными ей делами. Я уже предлагал посмотреть, как можно переделать.
От вызова SetPartitioning() во время TxInit защищаться и не надо -- это ключевая вещь для продолжения процесса data erasure, если он уже работал до рестарта. Хотя конечно не хватает комментариев c описанием зависимостей в порядке загрузки состояния DataErasureManager и шардов таблиц.
до этой транзакции даже очередь не дойдёт
Это если schemeshard перезапустится?
Тогда статус data erasure будет IN_PROGRESS и как раз SetPartitioning() во время TxInit отработают и обновят актуальный список шардов для чистки. Логически верно, но кажется напряжно. Было бы лучше на рестарте выполнять один проход по общему списку шардов вместо отдельной транзакции на каждую таблицу.
Changelog entry
...
Changelog category
Description for reviewers
Return error in case of borrowed parts being present in DataShard. SchemeShard will retry these failed DataCleanup attempts.
In case of split/merge SchemeShard will wait old tablet deletion.