#51: Код ревью в Clickhouse

[00:00] Александр: Здорово! Меня зовут Саша Пахомов, и я инженер, который любит своё дело. Это 51-й выпуск подкаста «Тысяча фичей». Сегодня хочется поговорить про всем знакомый и обыденный процесс код-ревью. Обыденный настолько, что мы редко задумываемся: а зачем мы это делаем? А правильно ли мы это делаем? Что чувствует человек по ту сторону пул-реквеста? Обсуждать код-ревью в вакууме — это не про подкаст «Тысяча фичей», поэтому я позвал коммитера ClickHouse Максима Кита. Максим наверняка знаком вам по предыдущим выпускам — про ClickHouse, C++ и оптимизации. Он проревьюировал порядка тысячи пул-реквестов, и ему точно есть что рассказать. Проект ClickHouse мержит в апстрим почти все пул-реквесты — как у них это получается? Завариваете чаёк, выходите на прогулку или делаете то, что обычно делаете во время просмотра. Поехали!

[01:26] Александр: Какой процесс код-ревью самый общий? Что ты под этим вообще понимаешь — чтобы было понятно, про что мы говорим?

[01:33] Максим: Конечно, всё очень сильно зависит от специфики проекта. Большую часть серьёзной open-source-разработки я делал именно в ClickHouse, и там код-ревью — это была required-часть перед тем, как пул-реквест будет смержен. В обычном проекте — каком-нибудь простеньком UI на JavaScript — код-ревью, возможно, не такая уж серьёзная часть, потому что там в целом можно просто глазами просмотреть. Например, была задача добавить вьюшку, поменять кнопку — концептуально всё понятно, что человек делал, поэтому ты можешь просто быстренько пролистать код глазами, и всё в принципе ок, можно мержить. Такое ревью много времени не занимает. А в ClickHouse код-ревью может занимать даже больше, чем сама разработка. Под код-ревью я имею в виду не чисто время просмотра кода, а абсолютное — со всеми пинг-понгами: ты как ревьюер написал сообщение, человек ответил, и всё это растянулось на месяцы. Вообще говоря, это может быть просто обсуждение. Во всяких проектах — например, в Postgres — обсуждение это такая тяжёлая часть, где нужно действительно понимать, что происходит, там всё очень аккуратно. Я бы сказал так: ClickHouse не настолько серьёзен в плане код-ревью, как какой-нибудь Postgres, где прямо очень-очень долго обсуждают, как что-то сделать. Обычно, если приходит какой-то пул-реквест, его удаётся относительно быстро проревьюить — и уже мержить.

[03:14] Максим: Но в ClickHouse код-ревью тесно связано с CI. В open-source-проектах так в принципе всегда должно быть: есть CI, и он позволяет снять с код-ревьюера огромную часть ответственности. Приходит человек, сделал пул-реквест — а он, может, только недавно начал писать на C++, и у него сразу куча багов, куча крашей. Это всё ловится тестами, и он может зайти в пул-реквест, посмотреть: о, тут тесты упали. Он видит репорт, смотрит логи. Вот это обязательно должно быть в CI, если делается open-source-проект, — чтобы внешний человек спокойно мог пофиксить любую проблему и сам понял, в чём дело. А если он пингует, можно ему просто сказать: вот, какие-то тесты сломаны. Так оно и работает. Из моего персонального рекорда: я проревьюировал где-то тысячу пул-реквестов в ClickHouse за примерно шесть, может девять месяцев — плюс-минус три месяца, условно за год. А суммарно за всё время, наверное, удалось проревьюировать где-то полторы тысячи.

[04:56] Максим: И ещё, чтобы сказать глобально: код-ревью — это, конечно, мастхэв, если проект сложный. Казалось бы, могли бы всё отдать на CI: серо-серо, зелёно-зелёно — всё, смержили. Но по факту так делать очень плохо. Какие-то баги, понятно, можно отловить на код-ревью, но главная задача ревьюера — задавать вектор развития пул-реквеста. Ревьюер пришёл, спрашивает: зачем этот PR? То есть в идеале должна быть мотивировка — либо ишью, из которой сразу понятно, зачем это делается, либо сам автор пишет огромный комментарий: это пофиксил такой-то баг, и так далее. Я лично, когда делал ракетство, всякие пет-проекты, всегда пишу такое мотивирующее письмо — зачем я это сделал. И в идеале, если ты как разработчик подумал про несколько путей, как это реализовать, и выбрал один, — нужно расписать сразу несколько путей и сказать: я выбрал вот этот. Это уже скорее про общение между ревьюером и автором, тонкая вещь, её мы ещё обсудим.

[06:10] Максим: А главная задача код-ревьюера в open-source-проекте — не допустить, чтобы попал код, который не соответствует духу проекта. У нас в ClickHouse общая сложная кодовая база, и какие-то вещи мы делаем определённым образом. Если бы в кодовую базу начали тащить чужеродные паттерны из других проектов, код было бы намного сложнее понимать: у тебя есть кодовая база, и все файлы должны быть плюс-минус одинаковыми в плане стиля — style guide и всего такого. Это можно инфорсить на уровне линтеров, чекеров и тому подобного. Но есть вещь, которую тяжело заинфорсить, — это, даже сложно сформулировать, как у нас сделаны проходы оптимизатора, как делаются парсеры. Есть какой-то подход не к стилю в смысле «где запятые поставить», а к тому, как нужно выстроить pull request, чтобы всё было понятно. За этим тоже, конечно, ревьюер должен следить: понять, зачем этот PR, и в принципе как это должно быть реализовано.

[07:42] Максим: Есть такой антипаттерн: если ревьюер сам не понимает, как эту фичу нужно реализовать, то полноценного код-ревью не получится — ты просто не сможешь сказать ничего важного. Сможешь только посмотреть на стиль, как я уже сказал, но не сможешь понять, можно ли было это сделать как-то по-другому. Чтобы хорошо ревьюить в таких больших проектах, как ClickHouse, нужно уже много кода понимать: буквально любую фичу, которую люди приносят, ты должен уметь сделать сам. Человек сделал pull request, объяснил свою идею, как планировал это реализовать, — ты должен сразу понять, будет ли это работать, и проревьюить его код. Это, наверное, самое важное. А то часто бывает — прямо в больших open-source-проектах я не думаю, что такое часто встречается, просто там open-source-проект развалился бы, — но в компаниях сплошь и рядом: код-ревью делают люди, которые в этом коде вообще ничего не понимают. И это на самом деле такая опасная штука, потому что в таком случае реально легко засадить какой-нибудь баг. Например, был код, автор изначально написал его определённым образом, потому что знал какие-то детали. И вот эти детали часто указывают в документации, реже в комментариях, а бывает — нигде: человек должен из кода понять. Приходит другой ревьюер, поменял это место, а потом человек со стороны проекта, который вообще с этим кодом не знаком и планирует всё это смержить, смотрит: все запятые и пробелы стоят нормально, опечаток нет, смысл вроде есть, — а вот именно такие детали (может, тут вообще так не надо делать, может, тут какая-то подстава) он понять не сможет.

[09:35] Максим: В идеале многие вещи, про которые я сказал, очень хорошо форсить на уровне проекта. Например, хочется, чтобы код был структурированным, — это надо форсить в первую очередь на уровне интерфейсов, и для тех, кто работает на проекте, и для внешних людей. Очень хорошо, когда используются паттерны — например, factory. Это вообще прекрасный паттерн для open-source-проектов, где хочется задать какой-то общий интерфейс, а потом уже реализовывать его, не вдаваясь в детали конкретных классов. Вот в ClickHouse у нас почти всё построено на factory-паттерне: колонки — нет, но в большей степени, например, агрегатные функции, просто функции, движки баз данных, движки таблиц — это всё один интерфейс. Ты реализуешь его сбоку, аккуратненько в отдельном файле, конкретные методы — и всё, вся система сразу инжектится и начинает прекрасно работать. Люди, которые пишут код, должны писать его так, чтобы другие потом могли этот код легко менять. Вещь понятная, но на самом деле её очень сложно делать. Во многих open-source-проектах — вот если бы мы могли просто запрыгнуть в какой-нибудь проект, скажем, за час попытаться что-то поменять, — на самом деле очень мало проектов, где ты реально сумеешь это сделать. Например, на Java хорошие проекты вроде Spring: там всё хорошо сделано в плане того, что если тебе нужно что-то найти — очень понятно, в каком модуле оно лежит, какой класс затрагивает, куда в коде тебе нужно прыгнуть.

[10:57] Максим: Code review — это часть, где какие-то внешние люди пишут код; часть, как устроен CI; и часть, как сами контрибьюторы должны писать код, — всё это очень сильно взаимосвязано. То, что в ClickHouse очень много внешних контрибьюшенов, — это заслуга всех этих факторов: и того, что на код-ревью есть определённый паттерн, как общаются с людьми, которые приходят снаружи, и того, что CI должен быть офигенный, чтобы снять очень большую нагрузку, и того, что сами люди должны писать код правильно. Вот, наверное, много всего набросил, но как раз нам на весь подкаст.

[12:00] Александр: Да, набросил ты так мощно. Я думаю, проще всего, наверное, начать с CI: он такой понятный, на нём можно разогреться. Когда мы говорим про continuous integration — то есть про то, как мы интегрируем новый код в основной, по сути мержим пул-реквесты, — тут интересен именно угол, что CI — это помощник для код-ревьюера. Довольно интересный взгляд, потому что часто, когда мы про CI говорим, мы проговариваем одно и то же: должны быть линтеры, какие-то чек-стайлы, может, даже форматтеры, полуавтоматические тесты. Но мы это рассматриваем как что-то, что не позволяет откровенно неаккуратному коду пройти, или ловит какие-то очевидные баги — в Java, например, явный null pointer exception. То есть просто отлавливает баги и делает структуру чек-стайла проекта одинаковой. Когда мы это говорим, мы вроде понимаем, зачем CI. Но давай теперь посмотрим со стороны ревьюера. Представь, что CI вообще нет, и твоя задача — проревьюить какой-то код, который к тебе заслали, чтобы он был правильный. Сколько нужно интеллектуальной энергии, сколько думанья: локально позапускать тесты — CI-то нет, — форматтером пройтись, глазами всё прочитать, отловить какие-то мелкие баги. Насколько ревьюеру становится тяжелее, если нет просто зелёного CI, в котором есть всё? Может, на примере ClickHouse посмотреть, что там может быть кроме очевидных вещей, — поконкретнее, именно с точки зрения того, как это помогает на ревью.

[13:57] Максим: Да, давай поговорим. Даже та часть, где ты сказал про стайл-чекеры, — вот у нас первое, что запускается в ClickHouse, это стайл-чек. И если он не проходит, вроде бы остальные пайплайны не запускаются: может, запускается фаст-тест, но мощные пайплайны, где уже билды со всякими санитайзерами, вообще не стартуют. То есть сначала нужно поправить стиль. Это базовая вещь: человек взял файл, отформатировал, стиль поддержал. Всё можно запустить локально — важная штука, потому что даже в 2025-м есть проекты, где люди всё ещё иногда на код-ревью пишут: «отформатируй-ка это так», или «вот тут два пробела». Это всё лишнее для ревьюера. Я и сам не люблю, когда по коду много всяких стилистических изменений, — это отвлекает: поменялось десять строк, а по факту изменение одно, остальное — какие-то переносы. Если есть стайл-чекер, ты даже мозг не напрягаешь: всё, этот вопрос решён.

[15:09] Максим: Это на самом деле серьёзный вопрос. Например, в Go — чем мне и нравится Go — по этому поводу вообще нет дебатов: есть стандартный gofmt, ты код форматируешь и даже не паришься. Есть нюансы — на проектах всё равно могут приходить люди и инфорсить какую-то длину строк, можно понаходить детали, — но в целом этот вопрос решён. И в ClickHouse он решён на уровне стайл-чекера. У нас есть clang-format, но если ты просто отформатируешь файл clang-format, то в паре-тройке мест наш собственный стайл-чекер, который внутри ClickHouse, упадёт. Он сделан только для нескольких edge-кейсов. Этот стайл-чекер, которым всё проверяется, ты можешь запустить локально и проверить. Проблем с этим у нас в целом нет, но я к тому, что и этот момент можно доработать — чтобы прямо как в Go, одной кнопкой всё взял и заформатировал.

[16:09] Александр: Где, по сути, у тебя форматтер настолько продуманный, простой и понятный, что тебе проверять больше ничего не надо. У нас же есть две стадии: проверь, что соответствует правилам форматирования, — и отформатируй. Вообще говоря, это два разных действия, два разных процесса, две разные программы могут это выполнять. И когда эти две программы следуют немножко разным правилам в каких-то местах, становится проблематично: я у себя в редакторе настроил форматтер, он на сохранение файла или при коммите хуком всё форматирует, я отправляю — а оказывается, этот форматтер что-то отформатировал так, что чек-стайл на CI не пропускает. Эта проблема больше для контрибьютора, который приносит изменения. Ревьюеру-то что — он видит, что чек-стайл не проходит, значит, надо, чтобы проходил. Но вот эта синергия тулов — что-то, что, мне кажется, в Go и в Rust решено на уровне дизайна экосистемы языка. Это огромная заслуга современных языков. Потому что когда ты начинаешь работать в проекте, где это есть, — не просто «все говорят про gofmt, круто», а ты поработай в проекте, где gofmt в CI, где он может автоматически всё сделать, где у всех всё настроено, — разговор про формат как сущность вообще уходит, его нет даже в голове. И появляется больше пространства в голове, чтобы смотреть в суть кода, — не тратить топливо на «а здесь перенос норм или не норм, ну чек-стайл прошёл, значит, наверное, норм». Ты просто про это забываешь. И это действительно круто. К сожалению, в совершенстве такого нет в C++-тулсете, и в Java нет, и в Kotlin особо тоже нет — то есть JVM-часть языков тоже этому подвержена: все форматируют IDEA и что-то чек-стайлом проверяют. А расскажи про C++ — как это происходит именно с точки зрения тулов?

[18:25] Максим: Например, как у нас сделано в ClickHouse. Это не единственная тула, но такая мейнстрим-тула для таких вещей — это clang-format. Ты грузишь в неё формат, и всё прекрасно форматируется, вопрос на уровне clang-format в целом решается. Но не решается он в случае, когда, например, у нас в ClickHouse есть места с кучей лямбд или шаблонов, в которых clang-format… Так как это open-source-тула, в чём минус: ты сам должен взять правило и написать его для проекта. Обычно по опыту больших open-source-проектов у них уже есть predefined-файлы. Например, есть темплейт Google Chromium, есть темплейт WebKit. Ты берёшь этот темплейт и для своего проекта продумываешь, как хочешь сделать. По дефолту — когда я сам какой-то проект делаю, не парюсь, знаю, что я там один более-менее пишу код, — можно взять готовый стиль Chromium и не париться: всё информатировал, вопрос решён. Проблема возникает, когда хочешь под свой проект аккуратно что-то сделать.

[20:08] Максим: Такое ещё может происходить, например, в ClickHouse. Возможно, такой проблемы и не было бы, но изначально ClickHouse появился из Яндекса, а в Яндексе уже был какой-то стиль, заданный ещё, не знаю, в двухтысячных, когда, наверное, clang-format ещё не было. И потом, когда писали код ClickHouse, часть стиля пытались подстроить под общий стиль кода в Яндексе, а он в каком-то смысле уже немного устарел — есть детали, которые прям глаза режут. Когда делался изначально ClickHouse, скорее всего — да даже точно, часть кода есть — в разнородном, устаревшем стиле. И получается, когда стали использовать clang-format, скорее всего пришлось много всего подтюнить, чтобы за одно форматирование не пришлось переформатировать весь проект — просто чтобы историю файлов не ломать. Сейчас в Git есть специальные штуки, чтобы такие коммиты, где ты просто код форматируешь, глобально как-то скрывать, но я, честно скажу, глубоко не смотрел, как это работает, по работе такого не было. Поэтому у нас в ClickHouse есть свой конфиг clang-format, и вообще, к сожалению, почти у любого C++-проекта есть свой конфиг clang-format, в котором используется шаблон, но какие-то детали изменены. То есть у C++-разработчиков нет такого, что один стиль, — у всех разные стили.

[21:03] Максим: Методы camelCase или snake_case — это, короче, до сих пор нерешённый вопрос. Изначально snake_case — это когда есть нижнее подчёркивание, правильно? Таким образом написана стандартная библиотека C++. Поэтому логично, что в своём проекте, когда пишешь классы, ты не хотел бы делать snake_case, чтобы не путаться, — а может, наоборот, и хотел бы. Этот вопрос всё ещё не решён. В ClickHouse — и, мне кажется, в целом — достигли почти идеала: у нас есть clang-format, ты можешь весь код форматировать, но потом, когда им отформатировал, всё равно нужно запустить линтер. Он проверяет не только форматирование, он ещё несколько вещей проверяет, и упасть может только в тех нескольких местах, где твой формат не удалось настроить — просто не было таких опций. Это очень редкий случай — лямбды или много шаблонов, чтобы их нормально отформатировать. Короче, редкость. Конкретно на уровне ClickHouse вопрос с форматированием в принципе решён, но на уровне всей C++-экосистемы — вообще нет. Думаю, его никогда не решат, потому что уже так много кода написано. Вот представь код в LLVM — кстати, один из шаблонов это LLVM-код. Шаблоны в основном строились по major-проектам: Chromium, LLVM, WebKit, Mozilla вроде есть ещё какой-то. И на каждом проекте берут какой-то шаблон и всё-таки под себя его подгоняют. Ты переходишь с одного проекта на другой — и реально… Я большую часть времени, лет пять, писал код в ClickHouse, и когда прихожу на проект, где стиль вообще другой, это реально глаз режет, мозгу мешает: не можешь быстренько ориентироваться. Видишь в ClickHouse методы с большой буквы, CamelCase, а в каком-то проекте они snake_case, и думаешь: snake_case — может, это что-то связано со стандартной библиотекой? Вот это перестраивание немножко мешает. Конечно, если бы был как в Go один стиль, было бы прекрасно, но примерно такое состояние в C++-экосистеме.

[23:14] Александр: Когда Labo ещё писал на плюсах, я что-то помню, мы тоже не понимали, как правильно, — snake_case. Потому что обычно в языках программирования, почти во всех, уже устоялось: либо snake_case, либо CamelCase — либо с большой буквы, либо с маленькой, других вариантов особо нет. А тут можно и так, и так, и действительно часто встречается и так, и так. Наверное, к этому привыкаешь, могу предположить.

[23:44] Максим: Нужно быть готовым, что тебе встретится любой стиль. Просто ни к какому стилю не надо привязываться.

[23:52] Александр: Да-да-да. Я, кстати, раньше думал, что это что-то, что сильно решает. Но сейчас я прямо много программирую на Java и много на Rust, и переключаюсь между этими стилями, когда набираю код, очень быстро. Такое ощущение, как будто у меня даже нет переключения — как будто одна часть мозга программирует на Rust, а другая на Java, и они вообще не пересекаются. И им это на пользу — лучше не пересекаться. То есть привыкаешь, по крайней мере у меня так. Действительно очень разные стили написания кода, но что там, что там как-то просто уже шлёпаешь, и всё. Кстати, Copilot в этом плане сильно помогает. Но про AI-тулы, я думаю, поговорим в конце, потому что, мне кажется, в код-ревью они тоже начинают быть полезными. Так, ну окей — про формат, про стили, чек-стайл мы поговорили. Дальше у нас что?

[24:41] Максим: В ClickHouse у нас куча тестов. Есть stateless-тесты — те, что можно прогнать вообще без какого-то состояния базы данных: если надо создать таблицы, они сами их создадут и зальют туда какие-то рандомные данные. Есть stateful-тесты, для которых нужны данные, но эти данные лежат в S3, и таблицы мгновенно к ним подключаются и сразу с ними работают. Потом есть unit-тесты — это просто тесты в C++-коде: например, есть какой-нибудь свой массив, свой std::vector или своя структура данных, хэш-таблица, — это stateful-тестами тяжеловато прогнать, на всё это есть unit-тесты. И последняя часть — интеграционные тесты: там, например, поднимается ClickHouse, с ним может подняться Kafka, может подняться MinIO. Короче, когда несколько систем сразу тестируются: всякие сложные функционалы, коннекторы, базы данных, которые с Postgres как-то взаимодействуют или с чем-то ещё. Или несколько ClickHouse’ов может подняться — например, если тестируется реплицируемая база данных или реплицируемая таблица.

[26:00] Максим: Ко всему этому дополнительно есть ещё стресс-тесты. Стресс-тесты — это просто запуск stateful-тестов, но не по порядку: они как-то на группы делятся, и это всё запускается параллельно, а потом уже запускаются тесты, которые параллельно работать не умеют, — которые какие-то базы данных создают или у которых есть файловые dependencies. В стресс-тестах все тесты запускаются одновременно, и задача — чтобы в конце ClickHouse не упал. Они могут даже все не пройти, но главное, чтобы ClickHouse не упал. Во время стресс-тестов часто отлавливается много интересных багов: когда всё нафиг параллельно запускается, может начаться, я не знаю, куча дедлоков и всяких таких вещей. Все эти тесты мы запускаем со всеми возможными санитайзерами: у нас отдельный прогон debug-билда, отдельный release-билда, отдельный с address-санитайзером, отдельный с thread-санитайзером. С memory-санитайзером, по-моему… хотя, мне кажется, даже с memory-санитайзером мы отдельно гоняем, и отдельно с undefined-behavior-санитайзером.

[27:15] Максим: И для стресс-тестов есть ещё свой стресс-тест, где в примитивы синхронизации — например, mutex lock/unlock, в condition variable, wait/notify — мы вставляем джиттер, вставляем std::this_thread::sleep. Это нужно, чтобы лучше репродьюсились concurrency-баги. Ты запускаешь код с thread-санитайзером, а дополнительно во все примитивы синхронизации добавляешь sleep, чтобы тред засвитчить. Это помогает находить немножко больше багов в thread-санитайзере. Хотя в целом он и так должен находить, просто больше шансов, что что-то споймает. И ещё немаловажная вещь — это фаззеры. Фаззеры у нас есть на дата-форматы: это отдельная программа, которая запускает какой-то код в ClickHouse и, используя какую-нибудь библиотеку для фаззинга, генерирует данные, а затем пытается этими данными сломать какой-то код внутри ClickHouse. Например, код, как парсятся файлы, кодеки для сжатия-разжатия данных — это тоже можно так фаззить, и это у нас всё делается. И последнее — это перф-тесты. Перф-тесты у нас есть и под ARM, и под x86. Они, понятно, прогоняются только в релизе, без всяких санитайзеров, потому что иначе перформанс поедет. Вот это, в принципе, все проверки, которые есть в ClickHouse.

[28:48] Александр: Тут всё в принципе нужно. Такой кейс, когда проверок очень много, и тут есть несколько полезных вещей. Во-первых, если ты человек, который пишет код, и видишь, что CI зелёный, — очень маленькая вероятность, что у тебя есть баг.

[29:02] Максим: Она прям реально маленькая. В ClickHouse бывают случаи, когда смержили пул-реквест, — но, как я сказал, есть фаззеры, у нас, например, есть свой SQL-фаззер, который генерит странные SQL-запросы. Может быть, через несколько коммитов, когда он будет гоняться, он найдёт баг, который был заинтродьюсен в пул-реквесте с зелёным CI. Но это бывает прям очень редко, и баги такие обычно прямо edge-кейсы — не составляет никакого труда это пофиксить. Это не что-то, что ты, скорее всего, даже на ревью бы глазами не нашёл: какой-то очень странный SQL-запрос крафтится, или очень серьёзный edge-кейс, какой-то if, в который вроде как не должны были заходить, — в общем, где-нибудь выход за границу, например.

[29:50] Александр: Это про фаззеры, да. У них само определение фаззинг-тестов — это как бы статистическое тестирование: они генерируют входные данные разными алгоритмами и эвристиками, и данные от прогона к прогону могут быть разные. Просто не попалось такого входного edge-кейса, который вот в тот пул-реквест попал бы и увидел этот баг, а в каком-нибудь другом прогоне он статистически появится. С фаззерами это нормально — ты ожидаешь, что они могут не сразу найти баг, это допускается. А что насчёт флэки-тестов — знаешь, когда конкуренция: один поток заблокировался, другой нет? Часто на проектах бывают флэки-тесты, и это не фаззинг.

[30:34] Максим: Да, получается, у нас есть флэки-чек. Это когда ты добавляешь stateless-тест — есть такая отдельная проверка, которая просто пытается твой тест много раз запускать: грубо говоря, сто раз запустишь — сто раз пройдёт. Такое в ClickHouse есть. Я ещё хотел сказать: про фаззеры в целом в последний год, по-моему, или даже до этого добавили ещё вещь, которая собирает coverage в ClickHouse. Она пока не на максимум интегрирована, но в целом идеал, которого можно достичь: как я уже сказал, может быть, какой-то if, куда во время тестов мы не зашли, а фаззер потом сумел найти инпут, при котором в этот if зашли. С помощью coverage сейчас он строится, и можно посмотреть код, который реально написан, и понять, после того как мы прогнали все тесты, — зашли ли мы во все basic-блоки, во все if‘ы. Если зашли — всё ок; если нет — можем попросить человека написать дополнительный тест. Но, мне кажется, мы это на максимум не интегрировали, только потому что пока неочевидно, что оно будет очень хорошо работать. Я давно не говорил с коллегами, которые этот coverage добавляли; изначально была идея, чтобы на каждый пул-реквест строился coverage и смотреть, что для нового кода есть 100% покрытие. Но потом, возможно, от этой идеи отказались — хотя, может, сейчас это ещё как-то используется, я не смотрел.

[31:49] Александр: Да, про code coverage — это постоянная тема дебатов: нужен он, не нужен, искусственная это метрика или нет, должна ли она являться хардстопом и ронять CI из-за того, что у тебя coverage меньше чего-то. Всё зависит, мне кажется, от проектов и от людей, которые на них работают. Я, когда с coverage сталкиваюсь, если долго с ним работать, для меня это больше стоппер. Потому что часто на проектах — не таких, возможно, как ClickHouse в его текущем состоянии, но вообще, когда мы пишем код, — его иногда нужно реально быстро задеплоить. И даже с возможными багами и с возможным нулевым покрытием тестов мы эти риски на себя берём: нам надо сейчас задеплоить хэппи-пасс, чтобы работал, а остальное догоним следующим пул-реквестом. Это нормально, так бывает. А когда у тебя есть CI, который говорит: нет, нельзя, тебе нужно 80% покрытия, — ну вот и здрасте. Получается, что конкретные проверки начинают тебя тормозить. И тут непонятно, как соблюсти баланс. Мне кажется, в open-source-проекте нет такой острой проблемы, что надо что-то быстро сделать: там всё более размеренно, внимательно, своим чередом идёт эволюция кода. Но если чуть отойти от open source и уйти внутрь, в бизнес, — возможно, там CI и количество проверок должны быть чуть порасслабленнее. Что думаешь?

[33:40] Максим: Про coverage я сам скептически отношусь. По крайней мере по моему опыту: когда на проектах был какой-то coverage, люди пытались его забить какими-то тупыми тестами — чтобы в сейфы пробраться, — но толку с этого никакого, потому что 100% coverage не гарантирует, что твоя программа вообще работает.

[34:02] Александр: А я тебя здесь прибью. Не то что нет толку — есть толк, и он вредный. Потому что когда ты пишешь такие пустые тесты, ты делаешь так, что последующий рефакторинг этого места приведёт к тому, что эти бестолковые тесты упадут, и их нужно будет ещё раз переделывать. То есть ты замедляешь дальнейшую эволюцию такими тестами.

[34:21] Максим: Если тестировать какую-то структуру данных, конкретно такой ситуации, как ты расписал, скорее не будет: человек написал тесты, разные инпуты закинул, сортировку потестировал — тесты не повредят. А вот сценарий, где люди пытаются добиться 100% coverage каких-то более-менее интеграционных компонентов, где несколько классов должны между собой взаимодействовать, и это всё ещё на интерфейсах, приходится их как-то мокать, — вот тут может начаться безумие, особенно в Java, потому что там всё на интерфейсах часто делают, не переживают, что будет куча сложной иерархии, люди к этому привыкли. В C++ скорее не так; в Go, наверное, даже ещё меньше стараются делать абстракции, чем в C++. От проекта зависит, но в целом я про coverage: если делаешь какой-то сложный алгоритм, то даже если ты во все if‘ы зашёл — это 100% покрытие, но тебе ничего не даст. Какие-нибудь алгоритмы на графах: конфигурации графов могут быть совершенно разные, ты можешь разреженные графы или очень связанные закидывать, а какой-нибудь интересный граф не закинул. Чтобы на 100% быть уверенным, особенно в алгоритмах, нужно знать алгоритмы и очень чётко понимать, что каждая строка кода делает.

[35:50] Максим: Наверное, я бы сказал так: нерешённая проблема человечества — или хотя бы нерешённая проблема код-ревью — это как быть уверенным в своём коде. И чем глубже идти: можешь ли ты быть уверен в своём коде? Можешь. А в коде своих библиотек? А в коде ядра или компилятора? И ответа на это всё нет, потому что есть баги в ядре, есть баги в компиляторах — я с таким уже сталкивался. Поэтому людям, которые совсем уже заморачиваются, я бы скорее посоветовал расслабиться: нельзя написать код и сказать «всё гарантированно работает» — если только ты не какую-то embedded-штуку делаешь, вообще без ядра или с очень урезанной операционной системой, которую можно глазами две тысячи строк прочитать. А в современном мире быть уверенным, что твой код не крашнется, почти невозможно, потому что под тобой куча инфраструктуры.

[36:53] Максим: Тут в контексте код-ревью действительно хотелось бы отметить — можем чуть попозже проговорить, — что бывают пул-реквесты простые в том плане, что они могут быть даже концептуально сложные, но в них нет хардкорного кода. А есть самый плохой тип пул-реквестов для ревью — это когда в них есть хардкорный код: какой-то дикий алгоритм, очень-очень сложная оптимизация. А самые поганые для ревью — это вероятностные алгоритмы. Вероятностные алгоритмы — это, на самом деле, штука, на которой на ревью можно очень сильно зависнуть. В обычном алгоритме ты можешь код посмотреть, в голове пробежаться по разным путям, на Википедии открыть, подсчитать. С вероятностными ты тоже всё это можешь сделать, но если, например, человеку нужно какое-нибудь большое простое число или ещё какая-нибудь фигня, и он допустил опечатку — ты можешь это не споймать, и твой вероятностный алгоритм вообще перестанет работать.

[38:08] Максим: Я бы сказал, что самое сложное — именно когда в пул-реквестах какая-то сложная алгоритмическая часть: тебе нужно на 100% всё понять. Не может быть такого, что ты понял код на 90% и вроде бы ок, или даже на 95, — тебе нужно прямо на 100% понять, что алгоритм валидный. И тут, понимаешь, юнит-тесты обязательно, конечно, — чтобы на все базовые вещи было. В идеале, если код сложный алгоритмически… Вот у нас, например, в ClickHouse своя реализация хэш-таблицы, и один раз был баг во время ресайза. У нас написано, например, resize in place, и все знают, что это в принципе без проблем делается, но тебе в каком-нибудь учебнике не расскажут про edge-кейсы, которые там могут быть. А там есть один такой интересный edge-кейс — и хэш-таблица сломана. Или вот есть код, как удалять элементы из хэш-таблицы с linear probing, — этот код я писал в ClickHouse, и там на Википедии всё можно найти, псевдокоды и всё такое, — просто когда ты это переносишь, нужно всё очень-очень аккуратно сделать. Берёшь алгоритм как из текстбука, но этот алгоритм нужно засунуть в твою инфраструктуру, на твои структуры данных, твои объекты. И если ты что-то копипастишь или не уверен — вот зачем в этой структуре данных такой if, вроде бы понятно, но зачем он нужен? — «ну, вроде как по тексту надо, я его и оставлю». Это очень опасно.

[39:48] Максим: А вот с пул-реквестами, которые простые, часто бывает так: приносит человек какой-нибудь рефакторинг и говорит: я хочу отрефакторить интерфейс функции, по всему коду прошёлся, отрефакторил, и заодно реализовал свою функцию, для которой мне это было нужно. В таком случае ты можешь ему в пул-реквесте сказать: вынеси отдельно рефакторинг, отдельно потом функцию реализуй, — и это всё спокойно ревьюится, потому что понятно, что человек хочет делать. А вот низкоуровневые алгоритмы, оптимизации, всякие интринсики, где иногда люди добавляют префетчи, пытаются очень-очень низкоуровневые вещи оптимизировать, — там можно очень серьёзно попасть.

[40:34] Александр: С алгоритмами у меня есть тоже наблюдение, маленькая история. Я сам писал алгоритм для парсинга сиквела с помощью Pratt Parser — так называемый алгоритм, он такой полурекурсивный, очень требует визуализации в голове: ты прям должен представить, что берёшь в левую руку, что в правую, как ты эти деревья перерисуешь. И я это писал, и в какой-то момент всё нарисовал, у меня на час наступило прозрение — визуализация подгрузилась, я понял, что надо сделать, написал 90% парсинга всего, чего хотел: SELECT, INSERT, CREATE TABLE, — короче, базовый парсер сиквела, и оно просто прошло как по маслу. А потом, через час, я просто забыл. Вот сейчас я не в состоянии сесть и за час добавить парсинг чего-то нового крупного — если это не «был int, ещё давайте string добавим», а какая-то новая конструкция, — всё, мне надо заново его понимать. И в этом плане ревьюер находится в такой же ситуации, когда есть какой-то суперсложный для понимания алгоритм: его нужно понять, иначе можно его даже и не смотреть — это ревью, если ты не поймёшь.

[41:53] Максим: Это да, я на самом деле редко подобный код-ревью люблю, потому что, может быть, я его избегаю, чтобы… спасибо, не надо. У нас в ClickHouse такое было, у нас в Яндексе было дежурство — или вот было в консёрне дежурство, — где нужно было целый день отвечать на issues, на pull requests, ревьюить их. И вот когда есть какой-нибудь большой pull request на десять тысяч строк, там и алгоритмы будут, и супермного сложности, и как назло на твой день попало дежурство, когда он был сделан, и ещё какая-нибудь такая штука, которая капец как нужна: все такие «о, отлично, надо поскорее ревьюить, потому что к релизу надо успеть». То, что ты говоришь, что часто в open-source-проектах — как, например, в ClickHouse, — если залетает срок и хочется поскорее смержить: ты ждёшь три релиза. Вот это не про ClickHouse. Мы смержим его, даже если условно работает хэппи-пасс, а всё остальное не крашится, — нам главное, чтобы не крашилось, а дальше уже будем дорабатывать.

[43:02] Максим: Может, это плавно перетекает в разговор про взаимодействие ревьюера и человека, который делает pull request. Это не сказать, что какой-то научный анализ, это чисто мой опыт, — хотя по количеству pull request’ов я уже какой-то статистикой обладаю. Когда человек сделал pull request, ему, во-первых, приятно, когда ты сразу на него отвечаешь, как можно быстрее. И вот у человека какой-то запал, импульс, готовность работать над pull request’ом, — он с каждым днём, пусть будет, падает в два раза. Человек приходит: о, всё, я сделал классный pull request, я его сейчас запушу, — ой, не прошёл CI, — и он сейчас фиксит CI, отвечает на комментарий. Так проходит один, два, три, четыре дня, и в какой-то момент люди просто пропадают. Может быть, есть какая-то секретная константа, через сколько дней или часов у человека прям теряется запал. У меня самого куча pull request’ов в другие проекты, на которых у меня пропал запал даже пинговать, чтобы их выревьюили, или вообще заделывать. Есть несколько пул-реквестов в LLVM, которые я даже пытался себя заставлять доделывать, но как только понимаю, сколько там работы, — прям не хочется. И вот тут, как в ClickHouse, я старался делать так: человек делает pull request — и как можно больше давать ему фидбэка. Как только человек сделал pull request, нужно посмотреть весь код и дать как можно больше фидбэка — по стилю, но не по код-стайлу, а прям по стилю: как структуры выстроить, какие абстракции, дать советы, написать все мелочи, которые тебе не нравятся. Код-стайл-то не проверит, например, что метод принимает константную ссылку, — ну, типа, нафига мне ссылка? Пишешь такой чек, все детали выдаёшь, чтобы человек увидел, что ты реально код посмотрел. Человеку приятно, когда его код реально хорошо проревьюили, посмотрели все мелочи. И часто — например, для новичков, да и не только — мне всегда приятно, когда на код-ревью скажут что-нибудь, что сделает меня лучше: новую фишку узнаю, новую вещь мне расскажут. Это всегда приятно, и в целом приятно, когда процесс происходит быстро. Я старался именно так действовать — давать как можно больше фидбэка. Но это идеальный случай.

[45:50] Максим: А самые плохие случаи — как я со стороны человека, который просто пишет код в open-source-проект. Ревью может затягиваться, вообще говоря, на годы. У меня, например, был pull request в Spring, я делал его очень давно — четыре, может, пять лет назад. Он всё висел, висел, висел. Но, кстати, в оправдание скажу: человек, который проревьюировал мой код, — это было где-то год назад, — он заметил там какой-то нюанс, сказал: окей, я пофикшу этот нюанс, — сделал маленький коммит, пофиксил и всё, смержил. Мне даже не нужно было ничего делать. Приятно, когда это происходит. Но часто бывает антипаттерн: к тебе приходит человек, даёт код на ревью, а ты просто берёшь и ревьюишь по верхам — проходишь, смотришь: типы какие-то, имена в типах поправь, вот тут по ссылке передавал — передавай по константной ссылке, сделай const. Короче, не делаешь код-ревью, а делаешь какую-то непонятную вещь. Человек это делает, ты приходишь ещё, выписываешь пачку таких нитпиков — маленьких фиксов, — он ещё раз делает. И представляю, как думает такой плохой ревьюер: ну ладно, пора бы наконец проверить, — смотришь и говоришь: ой, ну твой код вообще, тут архитектурно всё неправильно. Но человек уже потратил пару дней, фиксил твои нитпики, а ты ему сейчас говоришь, что тут всё архитектурно неправильно, — но ты же мог бы это сразу сказать, потратить лишние двадцать минут. Мне кажется, это людям, которые код приносят, портит мораль, потому что ты по сути очень много времени начинаешь тратить: тебе сказали, что архитектурно что-то неправильно, и тот код, который ты до этого рефакторил, потому что тебя просили, теперь вообще не нужен. Мне кажется, это очень многих бесит, и, может быть, из-за этого они даже в open source не контрибьютят — потому что такой экспириенс. Какая может быть мотивация у человека делать пул-реквест в какой-нибудь ClickHouse? Научиться чему-то новому или сделать какую-то фичу, которая ему нужна. И если у человека нет мотивации делать конкретную фичу, он хочет реально что-то новое узнать для себя, — он приходит, а его начинают на код-ревью так вот пинать. Во-первых, код-ревью становится очень длительным; во-вторых, у человека падает запал, и он в какой-то момент просто уйдёт.

[49:53] Александр: Да, можно разные интересные словосочетания подобрать к тому поведению, которое ты описал со стороны ревьюера. Когда действительно в начале человек просто на пофиг смотрит, просто потому что надо, — например, он дежурный: ну мне надо, — что-то посмотрел верхнеуровнево, но мержить по-любому не готов. Человек вроде отписался, опять включился в контекст, поправил всё это, а потом оказывается, что ещё через пару итераций вообще нужно переписывать часть кода, которая до этого обсуждалась, но верхнеуровнево. Ну, типа, мудак ревьюер, я по-другому его не назову. И я бы тут даже сказал, что работа в офисе пошла бы на пользу процессу ревью. Потому что за такое предъявить можно: если бы со мной так коллега себя вёл, я бы раз, два, три, а потом сказал: слушай, чел, что ты делаешь, нахрена? А в open source ты никого не знаешь: какой-нибудь человек из другой страны пришёл, ты его в глаза не видел, аватарка странная, — он тебе и не предъявит.

[50:53] Александр: И объяснить по-пацански: слушай, — иногда это реально работает. Потому что это взаимодействие между людьми, тут нельзя полностью абстрагироваться, что вот мы инженеры, пишем код и мы неэмоциональные. Да нифига. Тот же самый запал, про который ты говоришь, — это же чисто психология. Либо у человека есть мотивация, желание, он получает удовольствие от того, что делает, и фигачит; либо этого желания нет, и тут, каким бы крутым инженером он ни был, — может, ему десять минут потратить, чтобы доделать, — но если ему нафиг не надо и у него эмоциональный дискомфорт при работе с этим проектом, он этого делать не будет. Поэтому, мне кажется, это сильно влияет, и нельзя про это забывать.

[51:23] Максим: Это важно, что ты про это сказал. Каждый раз, когда мы ревьюим и когда нас ревьюят, надо всегда думать, что на той стороне человек вообще чувствует.

[51:39] Максим: Что ещё, мне кажется, важно. Во-первых, нужно иметь мотивацию, если что, пул-реквест делать. Мне кажется, к этому подкасту мы как ещё пришли: когда мы обсуждали что-то на C++ Russia, я тебе говорил, что делал похожий разговор с людьми из Тарантула — очень давно, года три назад, может быть. Я с ними говорил про open source, просто рассказать, как у нас на ClickHouse всё устроено. И одна из вещей, которая людей удивила, — это то, что в ClickHouse рейт пул-реквестов, которые мы мержим, — 99 с плюсом процентов. Сейчас не могу точно сказать, потому что проект уже намного больше, но на тот момент он точно был больше 90 процентов, мне кажется, близко к 99. И тут важный момент: вот человек делал пул-реквест, пропал запал, — и что мы в таком случае делали. Мы, во-первых, брали пул-реквест, доделывали сами. И важный факт: мы не убирали авторство. Грубо говоря, в чейнджлоге — вот мы взяли пул-реквест, доделали, — и в чейнджлоге прямо указали конкретно автора, что вот этот человек это сделал. Ревьюер, если он ещё очень много доделал, мог себя указать как дополнительного автора. Почему это правильно делать? Потому что, вот представь, ты приходишь в какой-то проект, прекомнил, захотел какую-то фичу сделать, нашёл какую-то ишью, погнал её делать, потратил кучу времени, написал тесты, — а написать нормальные тесты ты можешь пару дней потратить, — ты этот код посмотрел, ты уже составил путь к решению проблемы. Это в принципе самое важное: код написать — вторичное занятие, а вот путь составить, где что поменять, чтобы всё аккуратненько было, — это самое важное. Ты уже почти весь код написал, и если что-то нужно потом доделать, ты всё равно сделал большую часть работы. Даже если не весь код написал, ты этим процессом — скорее всего, там ещё полчаса, полдискуссии — уже понятно, что вообще сделать, и это уже любой человек, или даже не человек… И когда я говорю «не человек», я не имею в виду плохого человека — я имею в виду AI, он тебе спокойно это всё доделает.

[53:18] Максим: То есть самое важное, когда конкретно человек приносит пул-реквест, — это понять, зачем он это делает, и понять, как это аккуратно сделать в проекте. Если человек уже почти весь код написал, и там, например, — часто такое по моей памяти в ClickHouse бывало, — у него не проходят тесты, пара каких-то edge-кейсов, которые можно за пару часов посидеть, поразбираться в коде, доделать, — и спокойно можно мержить. И в целом можно самому доделать. И вот на том митинге с людьми из Mail.ru, из Тарантула, мне кажется, это больше всего удивление вызвало — такое нетипичное для open-source-проектов. Вот я, например, сказал тебе про того человека из Spring, что он за мной доделал пул-реквест: вообще, я очень был доволен, потому что такое вообще не встречается. Чтобы человек взял за другого человека, написал немножко кода, ещё смержил и авторство человека оставил, — это очень редко бывает, и когда такое происходит, всем, мне кажется, мир становится лучше, как говорят.

[54:23] Александр: Какая-то культура внутри core-команды, которая мейнтейнит этот проект. Потому что, если такой культуры не будет, то ничего больше не пойдёт. И нужны ресурсы, ведь за разработку кто-то платит. Когда тот, кто платит, слышит, что ты за кем-то что-то делаешь и много времени тратишь на общение с какими-то людьми в интернете, которые прислали пул-реквест, он может резонно спросить: а тем ли мы занимаемся, если наша цель какая-то другая? Не конкретно про ClickHouse, естественно, а просто про какой-то проект. И вот про то, что на это нужно выделить ресурсы и понять, что выделение этих ресурсов оправдано, — на это тоже нужно решиться, посчитать, или просто иметь эти ресурсы. Мне кажется, мало какие проекты вообще способны себе такое позволить.

[55:20] Максим: Да, согласен. Ещё хотелось бы проговорить: до этого я сказал со стороны человека, который ревьюит код, а сейчас, наверное, могу сказать немного со стороны человека, который делает пул-реквест. Я сегодня как это раздвоение личности — и с одной стороны говорю, и с другой. Так вот, со стороны человека, который пишет код. Во-первых, я уже сказал: в ишью, в пул-реквесте, который создаётся, обязательно создать самому ишью и потом сделать пул-реквест, или прям сослаться на ишью, или написать огромную мотивировку, зачем ты это делаешь. И если были разные возможные пути, все их расписываешь. Дальше — к самому коду. Если в коде есть какое-то стрёмное или сложное место, часто бывает так: ты делаешь какой-то код, а в других местах могут быть какие-то утилитарные функции, предназначение которых так сходу понять довольно сложно, потому что они просто везде используются. Знаешь, как в каком-нибудь machine learning: в конце каждой функции вызывается условно какая-то функция, ты не понимаешь, зачем она нужна, но ты соответствуешь другому коду, взял и скопировал. Если ты сомневаешься — просто напиши рядом с этим кодом, на GitHub или GitLab, что вот я это скопипастил, по-честному: я скопипастил с другого места, потому что кажется, что во всех таких местах эта функция используется. Это очень круто.

[57:01] Максим: Потом, если по коду есть какое-то сложное место, но оно правильное, — ты этот код написал, знаешь, что он нужен, и понимаешь, что из кода его понять нельзя, — нужно попытаться как можно аккуратнее этот код переписать: переменные раздать получше, может, немножко поперемещать. Написание кода — это же как написание книжки: ты условно поаккуратнее написал какое-то предложение, кусочек кода. Если даже это не помогает — такое бывает, когда код связан с алгоритмами, — часто как ты ни распиши, даже если аккуратно переменные назовёшь, ты иногда закладываешься на какой-то очень сложный вариант, который сквозь весь твой алгоритм проходит. И понять конкретно в этом месте, например, почему ты не боишься выхода за границы массива, — иначе произошёл бы краш, — ты можешь написать: вот это такой-то вариант. И вообще, если код такой сложный, алгоритмический, обязательно нужно на всё написать комментарий. В целом лучше всегда писать комментарии, если надо. Если людей они будут бесить, вам скажут на ревью, — просто потому что, если где-то не нужен комментарий, ревьюер скажет, что не нужен, а если где-то нужен, ревьюер просто может не понять и, как я уже говорил, в зависимости от ревьюера может сказать: а, код фигня, я не буду это ревьюить, не готов это мержить, попробую завтра. В любом случае со стороны человека, который делает pull request, очень важно правильно свой код приготовить к ревью.

[58:34] Максим: Что часто меня раздражает — часто бывает так: человек сделал какую-то фичу, но при этом раскидал в каких-то местах логи, закомментировал их, когда девелопил, и делает pull request. И по факту нужно поменять три файла, но человек даже не поленился… то есть не поленился бы. Я сам, например, так делал: захожу, файлы прочитал, пролистываю и понимаю, что ничего лишнего у меня не закоммичено. А вот у него это не закоммичено правильно. И вот этот опыт — из-за чего у ревьюера может не понравиться, — потому что так можно очень много файлов вообще поменять: какие-то мелочи, где-то расставить какие-то логи. Это показывает, что чек, который делал PR, как-то не позаботился лишний раз пробежаться глазами. А это супербыстро делается: сколько там строк кода ты написал — быстренько пробегаешься, — и это очень легко: все давно лишние логи убрать, лишний код, может, форматирование добавил, ты там #include лишний добавил, который не используешь. У меня часто было на LLVM, представляешь, — правишь, добавлял какой-то класс и не использовал его нигде по ходу, видимо, во время девелопмента добавил, думал, может, этот класс полезен будет, а я сижу и реально пытаюсь понять, зачем. Сначала пытаюсь понять, зачем нужен этот класс: может, внутри какой-то синглтон или сайд-эффект происходит, из-за которого этот класс по факту нужен. Или если кода много, я не могу в GitHub быстро понять, используется этот класс или нет, — только Ctrl+C, Ctrl+F, то есть просто поиском.

[1:00:25] Максим: Это серьёзная проблема. Код, который вы делаете на pull request, нужно просто сделать супераккуратным: везде понимать, для чего нужна каждая строка, для сложных строк написать комментарий. Если было несколько вариантов, как какой-то алгоритм реализовать, можно, как я уже сказал, в комментарии на GitHub или GitLab, — но, если из контекста не совсем понятно, в какое место, — например, вы решили в каком-то месте использовать не стандартную вашу сортировку в проекте, а какую-то другую библиотечную функцию, — тогда лучше не в общем комментарии писать, а прям в коде: найти эту строчку и туда комментарий бахнуть, чтобы ревьюеру было понятно. Если всё это сделать, скорость ревью увеличится на порядок.

[1:01:19] Александр: Да, я про это даже как-то записывал часть выпуска подкаста — выпуск про self-review, по-моему. Это как раз привитие вот этих привычек: посмотреть на свои файлы просто в GitHub, открыть, — тебе с тебя не убудет, ты лишний раз будешь уверен, что ничего лишнего не накоммитил и сделал то, что хотел. Когда ты программируешь, дебажишь, логи какие-то принтами выставил, временную структурку завёл, чтобы удобнее было, — про это всё можно просто забыть, потом закоммитить, и, ну, теперь мячик на стороне ревьюера. А мячик можно перекидывать реально долго. С точки зрения целеполагания: чего мы хотим? Ты хочешь побыстрее, чтобы твой код попал в апстрим, значит, нужно минимум итераций пинг-понгов и максимальное представление информации на каждом шаге ревьюеру. И вот просто цепочка «зачем я это делаю» — ты несколько раз прокручиваешь это в голове и приходишь к тому, что реквест должен быть аккуратный, где нужны комментарии — там должны быть комментарии, ты должен его сам вычитать, понять, что ничего лишнего нет. К этому приходишь просто цепочкой размышлений внутри своей головы, то есть нет каких-то правил. Есть правило «пиши комментарий к коду»? Нет такого правила, но там, где это нужно, оно должно быть.

[1:02:44] Александр: Ну, тут такие капитанские, здравые советы, какие-то очевидные мысли, common sense, как говорится. Но, к сожалению, common sense не у всех common. И это ещё зависит от культурного контекста. Когда мы с чуваками в офисе и человек из твоей же инженерной культуры приходит, — примерно понятно, как коммуницировать и что для него результат. А вот человек из другой инженерной культуры придёт, коммит сделает — и для него результат это не то, что для тебя результат. И вот на этом стыке тоже может быть много проблем. Например, форматтер — я часто вижу, для некоторых людей это просто не результат. Результат — это код прислал, pull request выслал. А то, что он смержится или не смержится, это уже для некоторых может быть не таким важным, как для той культуры, в которой мы находимся. Тоже как вариант таких размышлений.

[1:03:38] Максим: Да, тут хорошо всегда представлять: если ты делаешь pull request, представить себя в шкуре ревьюера, а если ты ревьюер — представить себя в шкуре человека, который сделал этот pull request. Тогда часто можно понимать, что, например, пишешь код, и кажется, что если бы я этот код сам не писал и просто на него посмотрел, как будто в интернете увидел, — он какой-то странный, сложноватый, я бы там не разобрался. Или на уровне ревьюера ты можешь смотреть: о, человек сделал так. Часто очень полезно представить себя в шкуре человека, который написал код, чтобы понять: ты видишь, несколько файлов изменено, вроде бы идейно написано правильно, — зачем человек сделал это изменение? Ты можешь представить: а вот если бы я этот pull request делал, то, скорее всего, я бы для этого его сделал. Это помогает понять big picture: ты прошёлся по файлу, в принципе понял, — и это хорошо.

[1:04:42] Максим: Не знаю, правда, вряд ли тут будут люди, которым прям интересно, как ревьюить много кода, потому что, наверное, это редкая задача. Но одни из таких советов — тоже common sense: открываешь pull request, читаешь сверху вниз быстро, пытаешься понять, что там, но не зацикливаешься на мелочах. Самая большая проблема людей, которые хотят много кода ревьюить, — это то, что они могут зацикливаться на какой-то мелочи, на какой-то строке прицепиться и не понять вообще всё остальное, что написано, а просто написать: а зачем эта строка? — и всё, ревью прекратилось. А можно просто смотреть: вот этот метод какой-то сложный, он делает свою задачу, какой у него вход-выход, — можно мыслить в терминах входа-выхода: вход такой, выход такой, всё, мне на эту функцию пока всё равно, я потом с ней разберусь. Вот этот класс — какая у него задача, какой у него вход, что он в конструкторе принимает, как с ним работать, какой выход, какая функциональность. В таком случае намного проще делать ревью, потому что первая важная вещь — концептуально понять, что в пул-реквесте всё ок, что ты готов его мержить, просто нужно немножко аккуратнее написать.

[1:06:01] Максим: Потому что это то, что нужно сделать на первом ревью. Сделали pull request — вот это должно произойти. Если там, грубо говоря, нет каких-то принтов и всего такого. Если есть принты, в принципе, можно сразу сказать человеку переделать, потому что вполне возможно, что там будут куски кода, которые вообще не нужны, — тогда лучше человеку приходить уже на второй ревью, он пойдёт, поменяет на следующую итерацию. А на первой итерации самое важное — вообще этот пул-реквест смержишь ты или нет, грубо говоря: вот этот код, все твои замечания сейчас пофиксят, но нет ли никакой концептуальной дыры, что в таком виде это вообще никак не смержится. Часто бывает, что приносят пул-реквест, он не изолированный. Хотя, например, сейчас бывает так: принесли какую-то функцию — у нас в ClickHouse не было особо дискуссий, типа стоит ли эту функцию мержить или нет. Обычно понятно: если функция новая, её никак нельзя через другие функции аккуратно выразить, или если выразишь — очень сильно перформанс пострадает, то мы мержили. У нас не было такого, как, наверное, в каком-нибудь Postgres, где часто думают: вот эта функция — а как она повлияет на судьбу проекта потенциально? У нас в ClickHouse такого не было: функциональность нужна — мержим. Даже если она какому-то человеку нужна, оно в принципе изолировано, потому что, как я уже сказал, повсюду используется factory pattern, — значит, оно изолировано, это можно мержить, оно никак не сломает.

[1:07:34] Максим: Но, понятно, если прилетает pull request, который меняет что-то в репликации или в каком-нибудь формате данных, тогда, конечно, нужно думать, будет это смержено потенциально или нет. Потому что если нет — нужно писать человеку как можно раньше, что так не получится, тут концептуально оно так работать не будет, мы это не сможем смержить. Это очень важно. Можно даже не смотреть на этом этапе в детали каких-то изменений алгоритмов репликации, когда ты понимаешь, что верхнеуровнево это ломает вообще концепцию, заложенную в код, и мы это не смержим, — поэтому лучше сразу сказать: нет, тут не пойдёт, — и не тратить свои силы на то, чтобы разбираться в том, что всё равно не смержится.

[1:08:18] Александр: Да, именно так. То есть в каких-то моментах в ревью просто абстрагироваться от деталей и понять, стоит ли смержить это вообще или нет — конкретно текущую реализацию.

[1:08:32] Максим: Иногда, если человек делает… вот как я уже сказал, ещё первичная вещь: человек должен написать, что он делает и зачем. Потому что, если код концептуально соответствует, — это хороший код, на концептуальном уровне он свою задачу решает, — но тут ещё первично, вообще нужно ли эту задачу решать. Вначале нужна какая-то ишью, хорошая мотивировка, зачем этот код нужен, а уже потом этот код смотреть и понимать, соответствует ли он концептуально проекту, не разламывает ли он всё нафиг.

[1:08:55] Александр: Идеальный механизм взаимодействия ревьюера и контрибьютора такой. Сначала контрибьютор, инициатор, — очевидно, ему надо что-то внести, он увидел какую-то проблему, например, дебажил ClickHouse и увидел, что в какой-то момент он зависает, — создаёт ишью. Дальше в идеале не решает эту задачу, потому что вдруг ревьюер скажет: а у нас тут уже это решено вот в этом классе, или что-то другое. То есть он декларирует свою мотивацию, получает обратную связь, что да, слушай, надо делать, подтверждает: я сделаю. Делай, условно. Пошёл, сделал, подготовил реквест с кодом, с мотивацией, слинковал с ишью, отправил реквест, — и уже на этот момент мотивация понятна, была проговорена до, и все согласились, что это надо решать, и вот тебе решение. В этом плане максимально эффективно утилизируются ресурсы обоих участников.

[1:09:59] Максим: Тоже важный момент, что часто тебе действительно могут сказать, что эту проблему не надо решать. Ты даже код ещё можешь не писать, тебе уже в ишью ответят. Если ты, например, собрался писать больше, не знаю, 200–300 строчек кода, то лучше реально создавать ишью и там пингануть кого-то с проекта, спросить, нужно ли это. Часто что ещё хорошо: ты, например, начал какой-то код писать, если не уверен вообще, зайдёт людям или нет, — делаешь draft pull request. Просто основные идеи, какие-то абстракции создал, накидал, и делаешь draft pull request к какой-то ишью. А если ишью никакой нет, то это ещё важнее: если ты уже понимаешь, что вроде бы ишью нет, такой проблемы нет, а ты уже начал много кода писать, потенциально это могут зареджектить, — ты делаешь draft pull request, описываешь свою мотивировку и ждёшь, пока ревьюер что-то ответит. Он может сказать: да, твой подход ок, давай продолжай этот pull request делать. Это важно, чтобы не… Такое бывало в ClickHouse, что иногда люди приносят pull request — мне кажется, рекордные были по 40 тысяч строк кода, — которые, мне кажется, всё-таки не будут смержены, потому что они конкретно для себя сделали какой-то оптимайзер или ещё что-то. Надеюсь, для себя, потому что тогда этот код хотя бы какой-то смысл имеет. Потому что, если они это сделали с надеждой, что смержится, то это, конечно, нужно обсуждать: если ты хочешь очень много кода — по open-source-проекту, мне кажется, уже тысячи строк кода это прилично, — а если это уже 10 тысяч строк, то перед тем, как писать код, 100% нужно обсуждать. Да даже тысячи строк кода нужно обсуждать. Без обсуждения заранее лучше не пытаться писать много кода, потому что обычно ревьюеров очень пугает, когда пул-реквест большой. Большой пул-реквест может сразу всё настроение испортить ревьюеру — реально нужно настроиться, чтобы посмотреть 10 тысяч строк кода, в которых нужно разобраться, продумать, как бы ты решал эту проблему, всё ли тут ок. Это, конечно, очень тяжело.

[1:12:16] Александр: Да, да. Хорошо, мы, я думаю, пробежались по двум пунктам, которые затронули: CI мы проговорили, — кстати, про CI и про перформанс-тесты очень хорошо поговорили ещё в предыдущих выпусках подкаста, я прилинкую, — потом взаимодействие, как минимум с двух сторон рассмотрели. У нас есть третья большая история — я бы предложил, возможно, недолго про неё говорить: именно про структуру кода, как должен выглядеть код, чтобы в него было легко внести изменения. Там можно очень долго разные техники обсуждать. Ты упомянул factory — думаю, это, наверное, самый известный паттерн, он плюс-минус во всех языках представлен: в Java это вообще классический, один из классических паттернов. Но я бы это подытожил так: способ создания объектов у нас вот такой. Мы не используем какие-нибудь конструкторы, инициализаторы и так далее — у нас factory, например, для такого рода объектов. И это уже действительно добавляет единообразие: сразу видишь — о, factory, значит, тут создаётся объект, вот как он создаётся, всё ясно-понятно. Может, есть ещё какие-то примеры, или хочешь ещё про factory поговорить, или какие-нибудь интересные техники компоновки кода, чтобы контрибьютить в него было проще?

[1:13:42] Максим: Ну да, давай тогда этот момент проговорим. Я могу сказать именно как за ClickHouse, потому что если у вас какой-то бизнесовый проект, вам этот уровень гибкости может наоборот мешать. Это скорее для проектов, которые на long term, которые долго можно будет мейнтейнить. Конкретно factory у нас используется почти для всего: для функций, для агрегатных функций, для сториджей, для движков баз данных, для словарей — почти для всего. Как это всё реализовать, можно посмотреть на Википедии, или в «банде четырёх» есть такая книжка. Мы хотели изначально в ClickHouse добиться того, чтобы человеку, которому нужно написать какой-то код, — понятно, что этот паттерн просто структура кода, но вот идеал, которого мы во многих местах достигли, — он создаёт отдельный файлик, например, с функцией, даже header-файл создаёт и cpp-файл, то есть прямо ничего наружу не торчит. Он там спокойно реализовывает свою функцию, наследуется от класса IFunction, и в конце добавляет условно глобальный метод — зарегистрировать себя. Этот метод тоже наружу не торчит, он через этап линковки прокидывается между файлами. В принципе, можно и в header-файле его указать. И потом в factory-классе, в котором все функции реализовываются, — у нас это вроде бы singleton, — у него есть метод registerFunctions, который где-то дёргается один раз, скорее всего прямо на старте сервера. Ты эту функцию registerMyFunction вызываешь, и она в factory пробрасывается, регистрируется. Так же и со storage’ем, и со всем остальным. Красота этого подхода в том, что человеку нужно как можно меньше копошиться в деталях каких-то других классов. Если ему нужна какая-то функция для работы со строками — он смотрит: моя функция похожа на какую-то другую, я возьму её, скопипащу этот файл, немножко поменяю, дам другое имя, зарегистрирую как функцию, всё.

[1:16:02] Максим: Это такой идеал, к которому нужно стремиться: что твой код вообще в отдельном файле, он никак с другим кодом не взаимодействует, даже на уровне типов ничего сломать не может. Оно может крашнуться, понятное дело, но ничего такого не ломает — это прям суперидеал. Таким образом у нас, например, в ClickHouse сделаны интерпретаторы запросов: запрос приходит, и для каждого запроса свой интерпретер, он в разных файлах. У нас так парсеры сделаны: парсер ты добавляешь, регистрируешь, и он с другими парсерами не взаимодействует. Есть общие парсеры, которые, например, для выражений тебе могут понадобиться, или когда ты парсишь идентификатор, — вот они наружу выставлены, остальные попрятаны. Это скорее не factory дело, а именно в том, что ты хочешь как можно меньше кода вообще выставлять наружу, делать публично, ты хочешь как можно больше кода прятать, чтобы вообще никак нельзя было добраться. Можешь это делать через factory, можешь каким угодно образом, но главное, чтобы код был друг от друга отделён и не возникало проблемы, что вроде бы человек хочет простую вещь добавить — добавление функции это простая вещь, или какой-нибудь запрос распарсить, — и это всё делается очень изолированно.

[1:17:17] Максим: Это ещё удобно, потому что если ты всё модульно делаешь, то, предположим, мы захотели добавить какой-то запрос в ClickHouse. Мы делаем отдельным файлом парсер, зарегистрировали парсер. Отдельным файлом интерпретер. У нас интерпретер запускается сразу на AST, а уже для SELECT-запросов там аналайзер, планер и всё остальное. Ты делаешь свой интерпретер, зарегистрировал. Интерпретер получит то, что с твоего парсера в него выскочит. Ты делаешь type cast, потому что знаешь, что оно всё на уровне типов сойдётся. Работаешь со своим конкретным типом запросов в своём конкретном интерпретере, реализуешь конкретную логику — и всё, очень запрятано. Нет такого, что всё торчит наружу, это суперидеал. Конкретно в этом случае наружу, скорее всего, будет торчать тип самого запроса, который будет распаршен. Это такая деталь, мелочи: интерпретер и сам парсер могут не торчать наружу, а AST — сама структура запроса — должна торчать, потому что интерпретер в другом файле должен этот тип знать. Там могут быть разные поля в запросе: например, «создать таблицу» — там будет имя таблицы, будут колонки. Понятно, что тип запроса eventually нужно будет в какой-то момент понять.

[1:18:43] Максим: Потом, что может этот паттерн ломать. Если хочешь так вот прятать код — что этот паттерн ломает, его ломают касты, особенно динамические касты. Вот, например, статический каст — как в данном случае, мы позволяем себе статический каст, потому что знаем, что наш интерпретер повязан на наш парсер. Но если тебе нужно делать динамический каст и там какой-то диспатч по типу — типа, у нас какой-то тип или другой тип, — то, скорее всего, в коде что-то не так. В ClickHouse есть такой код, например, когда мы реализовываем функции: туда приходит колонка, и мы там делаем dispatch по типам. Например, есть функция плюс, мы делаем dispatch левой колонки, dispatch правой колонки, чтобы все типы разобрать — все unsigned integers, все signed integers, все флоаты слева и справа то же самое, и это ещё может быть константный. То есть суммарно получается, например, 10 типов слева умножить на 20 справа плюс константы — ну, 400 будет специализаций. Но этот код обязан так делать только потому, что хочет выжать максимум перформанса. В целом можно было бы у каждой колонки реализовать функцию «плюсик» и в цикле её виртуально дёрнуть, но так не делается — это чисто для перформанса. Тут нет такого, что, знаешь, как серебряные пули: мы делаем так, и всё.

[1:20:04] Максим: Но, например, если у вас какая-то таблица, и вам нужно в каких-то местах дёргать не для перформанса, а просто понять, что это тип конкретно вот этой таблицы, — такой специальный, и из этого получается специальный код, — вот это знак, что, скорее всего, что-то не так. Скорее всего, нужно дополнительно какую-то абстракцию вводить, чтобы код был более общий. Потому что часто из этого потом очень сложно что-то добавлять. Представьте, у тебя есть какой-то специальный тип таблицы, ты в каком-то, например, InterpreterSelectQuery этот специальный тип таблицы каким-то специальным образом обработал, и потом, когда у тебя что-то поменялось, очень сложно следить по всему проекту, сколько там было сделано всех этих динамик-кастов. Если ты это делаешь на уровне интерфейса — например, на уровне интерфейса таблицы заэкспозил какие-то методы, — в ClickHouse есть, например, можно спросить: эта таблица вообще MergeTree, то есть она вообще хранит данные на диске? Но у нас уже с данными на диске можно, например, дёрнуть метод getDisk: если он nullptr вернёт, значит, диска нет. Какой-нибудь вспомогательный метод есть. То есть всё делать на уровне таких методов в интерфейсах — это хорошо, лучше, чем динамик-касты, и не бояться, что интерфейсы разрастутся. У нас, например, есть очень длинные интерфейсы — куча-куча методов, — но ты всё равно нигде не боишься, потому что состояние этого кода… Ты реализовал метод, его requirements, что от него ожидалось, все инварианты, — и можешь не бояться, что твоя таблица где-то с каким-то кодом не срастётся. Ты добавляешь новую таблицу, все методы реализовал, — например, есть метод isRemote, это для таблиц, которые работают на других серверах, умеют какие-то взаимодействия делать, — всё реализуешь, и всё хорошо работает. А вот если бы такого не было, представь: ты добавил один движок таблиц, и там, где тебе нужно знать, работает эта таблица с ремоут-сервером или нет, ты её скастил. У тебя появилась другая таблица, которая то же самое хочет делать, ей тоже нужна специальная логика, — ты этого не делаешь, потому что в том месте скастил, а для новой таблицы не сделал, — всё, всё сломалось.

[1:22:22] Александр: А в каких случаях, кстати, это может быть полезно?

[1:22:24] Максим: Ну, например, представь, ты выполняешь запрос, у тебя есть функция, называется hostName. Если у тебя нет никаких ремоут-таблиц, то есть запрос выполняется только на твоём инстансе, локально, — то ты сразу можешь это превратить в константу. Но если запрос будет выполняться на ремоут-инстансах, то, понятно, на другом инстансе hostName будет другой — это уже не константа. И ты мог бы, например, такой код: посмотреть, какие у тебя есть таблицы, ты знаешь, что у тебя пока что одна ремоут-таблица, ты её конкретный тип скастил — типа вот у неё такой-то тип, значит, у нас запрос с ремоута. Но по факту, если бы добавилась другая таблица, которая тоже с ремоута, то как этот запрос куда-то пересылать будет — у тебя этот код уже бы не работал, сразу же создался бы баг. Вот это тоже важно.

[1:22:58] Александр: Да, мне кажется, подытожить можно всё, что ты говорил, таким очень правильным и хорошим, мне кажется, одним из принципов вообще построения софта — такого, который долго будет жить, расширяться, дорабатываться, функциональность новая добавляться. Мне кажется, это принцип open-closed: когда у тебя система должна быть открыта для расширения, но закрыта для внутренних модификаций. Это ещё из каких-то древних принципов, может, даже в «банде четырёх» про это пишут, или это одна из букв SOLID — я уже точно не помню. Но я всегда исследую там, где нужно, потому что нельзя везде всегда код в таком стиле писать — он просто будет сложнее, у тебя структурки другие. Но вот когда у тебя есть какие-то функции, которые используются в сиквеле, и ты можешь передать туда данные, вызвать эту функцию, — но этих функций в базе данных, понятное дело, будет много, и со временем всё больше, больше. Поэтому система должна быть открыта для расширения набора этих функций, но закрыта — вся внутренняя кухня того, как эти функции передаются между собой, как они появляются в нужном месте. Должно быть легко расширить систему, но для того, чтобы её расширить, нужно по минимуму вникать во внутрянку и уж тем более модифицировать её, чтобы добавить какую-то новую простую функцию. И мне кажется, вот в тех местах, где это нужно, — например, command-line tools, где много разных команд, — добавить новую команду должно быть так же легко, как добавить новую функцию или новый rest-endpoint. Точки соприкосновения должны быть минимальными. И хорошо, что ты подчеркнул: одним файлом. С точки зрения код-ревью добавить новый файл всегда проще: ты берёшь — о, новый файл, давай сверху вниз читаем, — а не вкрапления какие-то того же кода в уже существующий код, это сильно сложнее. Поэтому если есть возможность отдельным файлом или хотя бы отдельным классом, каким-то кодовым юнитом добавить изменения, — это, мне кажется, хорошо, и с точки зрения ревью это как раз упрощает и увеличивает вероятность и скорость интеграции новых изменений.

[1:25:31] Максим: Да, я, может, ещё добавлю, что вообще, когда мы говорим про паттерны, — я уже несколько раз упомянул, я всегда боюсь это слово говорить, потому что оно часто срывает людям голову, и они начинают повсюду пихать кучу всяких паттернов. Вот, например, в ClickHouse я могу сказать, что у нас уровни иерархии очень маленькие. Например, есть IStorage, от него может быть один-два уровня — какая-нибудь таблица, и от неё ещё subclass, ну, максимум три-четыре уровня иерархии. А вообще паттерны часто, например, в той же «банде четырёх», появляются, когда нужно, например, разный код связывать, а тот код вообще поменять нельзя — это, например, в Java или где угодно. Есть паттерны, которые только для этого и созданы, например, адаптер: ты один интерфейс адаптируешь к другому. И вот в реальном коде это нужно очень осторожно делать. Потому что почему тот первый интерфейс вам нужно куда-то адаптировать, если вы можете его поменять? Может, лучше его поменять. Всякие декораторы нужно очень аккуратно применять, потому что легко, когда пишешь такой абстрактный код, начать увлекаться этим и на любой чих фигачить декораторы, адаптеры, всякие сложные штуки, всякий конь dependency injection. Можно очень сильно упороться, и код будет вообще невозможно понять, потому что абстракции по факту могут быть по сложности не меньше, чем просто сложный алгоритм: в голове сложную иерархию абстракций держать тоже тяжело — понять, что конкретно происходит, тяжело, нужно как-то в голове проинтерпретировать сложную абстракцию. Поэтому создавать новые типы, особенно типы-обёртки, нужно, наверное, только если тот тип, который вы пытаетесь обернуть, никак поменять не можете. Вот сейчас, например, есть клиент ClickHouse, и, наверное, можно сделать адаптер, чтобы key-slip как-то адаптировать, — ну, сама функция и будет этим адаптером. А вот если у вас есть какой-то код, и вы решили: о, я хочу его связать с другим кодом, сделаю какой-нибудь адаптер, — это очень опасно, потому что код очень легко можно сделать так, что там будет адаптер, декоратор, синглтон, всё подряд, и совсем ничего не понятно. Сомнительная затея всё это делать, если есть возможность не делать: вносить дополнительную сложность, дополнительный уровень абстракции, — даже если он всем понятен, — опять же, многие могут понимать вещи по-разному, людям надо вникать.

[1:28:34] Александр: Да, тут я полностью согласен. Мне кажется, мы хорошо прошлись по пунктам и не сильно забуривались в детали — прям на должном уровне прошлись. Мы вроде договаривались довольно мягенько записаться, потому что до этого были у нас выпуски, где, не знаю, две банки Red Bull, и надо в прогулку в лес с наушниками идти и слушать несколько раз, чтобы понять, — потому что для подкаста детали сложно обсуждать. Я бы на этом на самом деле заканчивал. Максим, у тебя есть что-то, что бы ты хотел ещё подсветить?

[1:29:12] Максим: Наверное, нет. Все основные пункты, мне кажется, мы довольно хорошо проговорили.

[1:29:16] Александр: Да, всем большое спасибо, кто слушал. Все ссылочки на предыдущие выпуски с Максимом будут в описании. И всем пока.

[1:29:24] Максим: Всем пока.