Implemented schema versions normalizer #9627
Conversation
|
⚪
*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 |
|
⚪ ⚪
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
| } | ||
|
|
||
| std::vector<INormalizerChanges::TPtr> changes; | ||
| changes.emplace_back(std::make_shared<TNormalizerResult>(std::move(unusedSchemaIds), maxVersion)); |
There was a problem hiding this comment.
а зачем maxVersion? зачем мы, вообще, последнюю версию в списке unused оставляем?
There was a problem hiding this comment.
Какая-то версия же всегда нужна
И будет правильно если она последняя
There was a problem hiding this comment.
так и зачем она из unused не удаляется?
There was a problem hiding this comment.
Она не в списке unused, а вторым параметром идет
| using namespace NColumnShard; | ||
| NIceDb::TNiceDb db(txc.DB); | ||
| for (auto& key: VersionsToRemove) { | ||
| if ((!LastVersion.has_value()) || (key.Version != *LastVersion)) { |
There was a problem hiding this comment.
Зачем кажды раз проверять. Может лучше LastVersion не помещать(удалить) из VersionsToRemove?
There was a problem hiding this comment.
LastVersion еще не известен во время накопления вектора
There was a problem hiding this comment.
можно удалить перед формированием задачи, или удалить в отсортированном векторе последний, если он равен, или, просто, удалить последний в отсортированном векторе.
|
⚪
🟢
*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 |
| for (size_t start = 0; start < unusedSchemaIds.size(); start += 10000) { | ||
| std::vector<TKey> portion; | ||
| size_t end = std::min(unusedSchemaIds.size(), start + 10000); | ||
| portion.insert(portion.begin(), &unusedSchemaIds[start], &unusedSchemaIds[end]); |
There was a problem hiding this comment.
обращение за пределами вектора - ub, насколько я помню. Перепиши, плиз, на итераторы
Напр. так:
std::vector portion{unusedSchemaIds.cbegin()+start, unusedSchemaIds.cbegin()+end};
There was a problem hiding this comment.
Заодно там же и фильтрую последнюю версию
| return std::nullopt; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Вот тут уже известен maxVersion и его можно удалить из unused
There was a problem hiding this comment.
Ну это же лишняя операция будет
Это же вектор, его нужно полностью обойти, а он и так будет обходиться в момент удаления
There was a problem hiding this comment.
а что мы оптимизируем?
|
⚪ ⚪
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
| @@ -59,6 +59,7 @@ enum class ENormalizerSequentialId: ui32 { | |||
| EmptyPortionsCleaner, | |||
| CleanInsertionDedup, | |||
| GCCountersNormalizer, | |||
| SchemaVersionCleaner, | |||
There was a problem hiding this comment.
Давай не будем его автоматически запускать, только в repair режиме
There was a problem hiding this comment.
Не, можно просто отсюда константу убрать
aavdonkin
force-pushed
the
schema_version_normalizer
branch
from
061c822
to
cbfe631
Compare
|
⚪ ⚪
🟢
*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 |
| @@ -308,10 +341,15 @@ Y_UNIT_TEST_SUITE(Normalizers) { | |||
| TestNormalizerImpl<TPortionsCleaner>(); | |||
| } | |||
|
|
|||
| Y_UNIT_TEST(SchemaVersionsNormalizer) { | |||
| TestNormalizerImpl<TSchemaVersionsCleaner>(); | |||
There was a problem hiding this comment.
локальные модификаторы были сделаны для моделирования ошибки в локальной базе. для случая, который у нас, можно, просто, поменять схему несколько раз. чтобы по ходу дела не удалялись схемы - через контроллер, просто, отключим, когда будет что отключать, и все.
There was a problem hiding this comment.
Тогда как проверить что нормалайзер отработал?
Сейчас я добавляю невалидную схему и если нормалайзер ее не удалит, то тест свалится с ошибкой
| } | ||
|
|
||
| std::vector<TKey> portion; | ||
| portion.reserve(10000); |
There was a problem hiding this comment.
кажется, что это можно убрать
There was a problem hiding this comment.
Почему?
Там как раз до 10000 элементов может добавиться
There was a problem hiding this comment.
Или отсюда убрать или после move'a тоже добавить. Сейчас как-то несимметрично. Для первого батча делается reserve, для всех последующих - нет.
|
⚪
🟢
*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 |
Changelog entry
...
Changelog category
Additional information
...