-
Notifications
You must be signed in to change notification settings - Fork 740
Extract methods to format records with rows #19141
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
|
🟢 |
ydb/core/base/table_index.cpp
Outdated
| auto copy = record; | ||
| TStringBuilder result; | ||
| // clusters are not human readable and can be large like 100Kb+ | ||
| result << " Clusters: " << copy.ClustersSize(); |
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.
не странно, что Clusters/Rows: будет перед общим описанием, а не после?
| TClusterId SetPostingParentFlag(TClusterId parent); | ||
|
|
||
| TString ToShortDebugString(const NKikimrTxDataShard::TEvReshuffleKMeansRequest& record); | ||
| TString ToShortDebugString(const NKikimrTxDataShard::TEvSampleKResponse& record); |
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.
а тут не странно, что оно в table_index.h? может эти функции прямо в классы TEvSampleKResponse и TEvReshuffleKMeansRequest засунуть?
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.
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.
а, ну эти конкретно не там, там некие обёртки. но может в них как раз и засунуть?
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.
Имхо, лучше аннотировать потенциально большие поля в proto-описании и реализовать кастомный принтер. См. ydb/library/protobuf_printer
| TString ToShortDebugString(const NKikimrTxDataShard::TEvReshuffleKMeansRequest& record) { | ||
| auto copy = record; | ||
| TStringBuilder result; | ||
| // clusters are not human readable and can be large like 100Kb+ |
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.
ну там и правда не понятно зачем байты строк в логе читать
я бы вообще ожидал что такой атрибут будет из коробки для протобуф файлов, но что-то не нашёл и как я понял никто не умеет добавлять поддержку атрибутов чтобы их сразу учитывал Codegen дефолтных методов помнить то что надо для каких-то протобуфоф использовать всегда отдельный принтер как-то тоже стрёмно так что пока бы оставил решение "на коленке" |
текущее решение тоже подразумевает "помнить". |
|
⚪ DetailsTest history | Ya make output | Test bloat
⚪ DetailsTest 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 |
|
⚪ DetailsTest history | Ya make output | Test bloat
⚪ DetailsTest 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 |
всё так, в PR каких-то значимых улучшений нет, просто вынос хэлпера а сделать значительно лучше — значительно сложнее, так что предалгаю влить как есть |
Changelog entry
...
Changelog category
Description for reviewers
...