-
Notifications
You must be signed in to change notification settings - Fork 695
Extract password verification to a separate actor #19687
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Extract password verification to a separate actor #19687
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
⚪ 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 |
⚪ 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 |
<< " at schemeshard: " << Self->TabletID()); | ||
|
||
const auto& event = *LoginFinalizeEventPtr->Get(); | ||
if (event.NeedUpdateCache) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Есть ли тест на разные значения NeedUpdateCache?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Я ещё до ПРа хотел проверять, но внешнему наблюдателю кеш вряд ли заметить. Может есть мысли, как тест должен выглядеть?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Может @ijon подскажет, как и нужно ли писать такой тест
ctx.Send( | ||
Self->SelfId(), | ||
MakeHolder<TEvPrivate::TEvLoginFinalize>(loginRequest, Response, Request->Sender, "", /*needUpdateCache*/ false), | ||
0, | ||
Request->Cookie | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Предлагаю здесь вместо посылки сообщения выставить флаг и в DoComplete инициировать локальную транзакцию TTxLoginFinalize
(Self->Execute(Self->CreateTxLoginFinalize(...), ctx)
).
Посылка сообщения самому себе -- это хорошо и нормально. Но в случае, если schemeshard будет перегружен, то событие будет долго висеть в очереди и будет обработано с большой задержкой. То есть получается, что обработка закешированных логинов (которая должна бы быть быстрой) на самом деле зависит от загрузки schemeshard'а и может быть пропорционально медленной.
Инициирование локальной транзакции напрямую поможет проверке закешированных логинов не так тормозиться при перегрузке. Execute
тоже не выполнит транзакцию "прямо здесь", но это в любом случае будет быстрее, чем ещё и дожидаться прохода события через возможно загруженный mailbox.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Поправил, но передача параметров туда выглядит немного грязно – с реинтерпрет-кастом по сути. Не резолвлю пока комментарий этот.
…errors; add comments to source actorId; faster hash check
⚪ 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 |
Changelog entry
Password verification is extracted to a separate actor from
TSchemeShard
local transaction.Changelog category
Description for reviewers
Existing
TTxLogin
transaction does not make all login actions now, but checks if password cache may be used for login verification. If hash check is needed, TEvVerifyPassword event is emitted and is handled in a newTLoginHelper
actor. A new transactionTTxLoginFinalize
is used for sending response on aTEvLoginFinalize
event.