YQ-4253 added CPU limit per process in CS #17804
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 |
GrigoriyPA force-pushed the YQ-4253-v2-support-cpu-scheduler-in-CS branch from 9986d96 to ede33e0 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 |
GrigoriyPA force-pushed the YQ-4253-v2-support-cpu-scheduler-in-CS branch from a1bba59 to fe84592 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
⚪ 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 |
| } | ||
| | ||
| TCPULimits::TCPULimits(const NKikimrKqp::TEvStartKqpTasksRequest& config) { | ||
| if (const auto share = config.GetPoolMaxCpuShare(); share > 0) { |
There was a problem hiding this comment.
давай проверку share > 0 вынесем в VERIFY (потому что ожидания пользователя в этом ключе будут не очень понятны...)
и не пиши два оператора в одной строке - пропустить можно, пока читаешь...
There was a problem hiding this comment.
и еще - я, обычно, предпочитаю отдельно создать класс, а потом - вызвать DeserializeFromProto, чтобы можно было TConclusionStatus вернуть с пояснениями
There was a problem hiding this comment.
Share <= 0 сейчас означает, что лимит отключён (перенёс эту проверку в kqp_node_service.cpp)
There was a problem hiding this comment.
рекомендую более явно включать/выключать... никто, читая код, не сможет догадаться, что share=0 специальное значение.. но не настаиваю
| ActiveWorkersCount = 0; | ||
| ActiveWorkerThreads = numberThreads; | ||
| ActiveWorkersIdx.clear(); | ||
| for (const auto& worker : Workers) { |
There was a problem hiding this comment.
я бы тут выделил случай numberThreads >= MaxWorkerThreads
и отдельно второй - тогда во втором после цикла numberThreads должен был быть 0
и вычитать из него тогда worker.GetWorker()->GetCPUSoftLimit()
There was a problem hiding this comment.
Заменил вычитание на numberThreads -= worker.GetWorker()->GetCPUSoftLimit(); и добавил проверку:
AFL_VERIFY(std::abs(numberThreads) < Eps); Её можно в обоих случаях делать, так как numberThreads = std::min(MaxWorkerThreads, AmountCPULimit); (пока не понял зачем разделять случаи)
There was a problem hiding this comment.
разделять желательно, поскольку в самом частом случае, как мне кажется, мы не будем по циклу ходить
GrigoriyPA force-pushed the YQ-4253-v2-support-cpu-scheduler-in-CS branch from fe84592 to 3d1616b 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
⚪ 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 |
3 participants
Changelog entry
Added CPU limit per process in CS for integration with Workload Manager
Changelog category
Description for reviewers