-
Notifications
You must be signed in to change notification settings - Fork 647
Fix stats inside plan for full stats mode #8553
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
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 |
this->Send(Target, ResponseEv.release()); | ||
|
||
Request.Transactions.crop(0); |
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.
сначала crop, а потом Send. в request transaction AFAIK есть unboxed values и аллокатор, поэтому сначала нужно его зачистить а потом делать send, чтобы избежать обработки двумя акторами
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.
Если в transaction есть UV, то почему нет баиндинга аллокатора?
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.
Если в transaction есть UV, то почему нет баиндинга аллокатора?
потому биндинг есть в деструкторе.
⚪
🟢
*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 |
@@ -390,7 +390,8 @@ message TDqExecutionStats { | |||
uint64 FirstRowTimeMs = 13; // first result row timestamp, milliseconds | |||
|
|||
repeated TDqStageStats Stages = 14; | |||
repeated string TxPlansWithStats = 15; | |||
reserved 15; // repeated string TxPlansWithStats = 15; | |||
map<uint32, string> TxPlansWithStats = 16; |
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.
- Зачем добавлять ключ, если он нигде не используется? Я увидел толко это место, и там txId игнорируется (https://github.com/ydb-platform/ydb/pull/8553/files#diff-6cefa5a3f132f45ba6bf9ed88bd0d2fecbc1c27c9e09cbd8d6f91284ddce0601R2820)
- Tx type = uint32 только в kqp. В dq service он имеет тип string. Зачем прибивать конкретный тип в общем proto?
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.
Тогда верну, как было - не подумал посмотреть, где ещё используется и с каким типом ключей. С т.з. нашего кода могло получиться, что мы не знаем для каких транзакций и сколько раз уже добавлены планы. Сделаю более аккуратно с нашей стороны.
this->Send(Target, ResponseEv.release()); | ||
|
||
Request.Transactions.crop(0); |
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.
Если в transaction есть UV, то почему нет баиндинга аллокатора?
⚪
🟢
*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 |
(cherry picked from commit 35c8dfe)
This reverts commit 35c8dfe.
Changelog entry
Changelog category