-
Notifications
You must be signed in to change notification settings - Fork 626
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
YQ-4205 fixed query service config passing into SS #16208
base: main
Are you sure you want to change the base?
YQ-4205 fixed query service config passing into SS #16208
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 |
5d2a52f
to
6251501
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
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
@@ -244,6 +245,7 @@ struct TAppData { | |||
NKikimrProto::TDataIntegrityTrailsConfig& DataIntegrityTrailsConfig; | |||
NKikimrConfig::TDataErasureConfig& DataErasureConfig; | |||
NKikimrConfig::THealthCheckConfig& HealthCheckConfig; | |||
NKikimrConfig::TQueryServiceConfig& QueryServiceConfig; |
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.
В целом доставлять этот конфиг до schemeshard'а через AppData корректное, но кажется не очень правильное решение. Оно замазывает реальную проблему (возможно в schemeshard'е, а может и нет), которую надо искать и справлять.
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.
В этом PR решается следующая проблема:
- На старте таблетки SS в ней нету QueryService конфига, по этому все external sources запрещены (так как список AvailableExternalDataSources пуст)
- Есть определённая задержка, с которой приходит конфиг от config dispatcher (она может быть достаточно большой, чтобы пользователи успели заметить неожиданные ошибки, что external sources отключены, хотя в конфиге они разрешены)
- Чтобы исправить эту проблему (конфиг должен быть сразу актуальным), пробросил его через AppData
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.
schemeshard не переходит в рабочий режим, пока не получит актуальную конфигурацию. И по-идее не может выполнить запрос, пришедший от внешнего с точки зрения ydb клиента на default конфиге.
Мне не понятно, как именно и в какой момент YQ пробует создавать resource-pool — может быть он это делает как-то слишком рано. Может быть, после создания базы YQ должен дожидаться рабочего статуса базы (как это делает ydbcp).
Может быть, в schemeshard есть какая-то недоработка, позволяющая просочиться слишком ранним запросам.
Это решение можно допустить временно, как заплатку, но, по-хорошему, надо искать дальше (и найдя, от этого проброса через AppData отказаться).
⚪ 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 |
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.
Замечание про QueryService
остаётся, оно касалось всех употреблений имени, а не только вывода в лог.
@@ -244,6 +245,7 @@ struct TAppData { | |||
NKikimrProto::TDataIntegrityTrailsConfig& DataIntegrityTrailsConfig; | |||
NKikimrConfig::TDataErasureConfig& DataErasureConfig; | |||
NKikimrConfig::THealthCheckConfig& HealthCheckConfig; | |||
NKikimrConfig::TQueryServiceConfig& QueryServiceConfig; |
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.
schemeshard не переходит в рабочий режим, пока не получит актуальную конфигурацию. И по-идее не может выполнить запрос, пришедший от внешнего с точки зрения ydb клиента на default конфиге.
Мне не понятно, как именно и в какой момент YQ пробует создавать resource-pool — может быть он это делает как-то слишком рано. Может быть, после создания базы YQ должен дожидаться рабочего статуса базы (как это делает ydbcp).
Может быть, в schemeshard есть какая-то недоработка, позволяющая просочиться слишком ранним запросам.
Это решение можно допустить временно, как заплатку, но, по-хорошему, надо искать дальше (и найдя, от этого проброса через AppData отказаться).
Changelog entry
YQ-4205 fixed query service config passing into SS
Changelog category
Description for reviewers