YQ-3561 add features for FQ-run #14826
Conversation
|
🟢 |
|
⚪ Test history | Ya make output | 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
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
GrigoriyPA
force-pushed
the
YQ-3561-add-features-for-FQ-run
branch
from
a66bbe9 to
887af1f
Compare
|
⚪ Test history | Ya make output | 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 |
|
⚪ Test history | Ya make output | 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
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
|
|
||
| fqrun_pid=$(pgrep -u $USER fqrun) | ||
|
|
||
| sudo perf record -F 50 --call-graph dwarf -g --proc-map-timeout=10000 --pid $fqrun_pid -v -o profdata -- sleep ${1:-'30'} |
There was a problem hiding this comment.
А нужно ли тут sudo? (возможно, в некоторых случаях -- да, но в некоторых случаях -- тот же пользователь, отсутствие параноидальных настроек с ptrace -- работает и без него). (Насколько я понимаю, это просто скопировано из kqprun, так что те же вопросы можно адресовать и к тому скрипту)
There was a problem hiding this comment.
Иногда без sudo у меня perf ругался, оставил чтобы работало скорее всегда (если прав на sudo нет, что редко, можно в ручную убрать).
Это ведь не засоряет лишними файлами fs для супер пользователя?
There was a problem hiding this comment.
perf record вроде ничего трогать не должен, и не сильно страшен; только вот он сохраняет perf.data с 0600 root:root, что побуждает и perf script запускать с sudo, а он делает всякое-страшное (и насчёт загрязнения не уверен)
В общем, чем с меньшими правами всякое запускается -- тем меньше шансов случайно отстрелить.
Я бы скорее вставил в скрипт что-то вроде $SUDO perf [...] и оставил выставление SUDO=sudo ./flame-graph.sh на пользователя (если у него без sudo не работает)
Но можно оставить и как есть
There was a problem hiding this comment.
Хорошее замечание, поправлю в следующем PR
| @@ -20,6 +20,7 @@ PEERDIR( | |||
| ydb/library/security | |||
| ydb/library/yql/providers/pq/provider | |||
| ydb/tests/tools/kqprun/runlib | |||
| yql/essentials/minikql | |||
| ) | |||
There was a problem hiding this comment.
рекомендую сортировать списки. util лучше убери, он всегда неявно подключается
There was a problem hiding this comment.
Я стараюсь везде сортировать список PEERDIR по возрастанию, где тут нарушился порядок? С ходу не видно
There was a problem hiding this comment.
ну вот ты добавил строчку не по алфавиту
There was a problem hiding this comment.
util тоже не по алфавиту был
|
⚪ Test history | Ya make output | 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
🟢
*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
Added features for FQ-run
Changelog category
Description for reviewers