Comments 28
Интересно, почему в описаниях проектов Яндекса (и связанных с ними) общепринятые в индустрии решения так часто выдаются за инновацию. Вы описали обычное, стандартное peer review. Практика эта хорошо формализована, и все более-менее продвинутые ребята так делают, причём давно.
Вам запрещено брать чужой опыт, и поэтому вы вынуждены додумываться до best practices самостоятельно? Это очень странно.
Вам запрещено брать чужой опыт, и поэтому вы вынуждены додумываться до best practices самостоятельно? Это очень странно.
А ребята автоматизировали это, получили отчёты и поделились.
В чём проблема?
В чём проблема?
Так проблемы-то как раз никакой нет %) Особенно её не должно быть для такой серьёзной на вид конторы.
А основная странность — в том, что заняться этим они решили только сейчас. Обычно к автоматизации peer review приходят ещё в течение первого года, когда команда вырастает до ~15 человек, и надо выстраивать процессы, чтобы завалов с PR не было. Тут же явно припозднились, яндекс-деньги сколько уже существуют?
Но так-то я рад за них. Лучше иметь хорошие процесссы поздно, чем никогда.
А основная странность — в том, что заняться этим они решили только сейчас. Обычно к автоматизации peer review приходят ещё в течение первого года, когда команда вырастает до ~15 человек, и надо выстраивать процессы, чтобы завалов с PR не было. Тут же явно припозднились, яндекс-деньги сколько уже существуют?
Но так-то я рад за них. Лучше иметь хорошие процесссы поздно, чем никогда.
Проблемы обычно решаются по мере их появления и обнаружения. При взгляде извне проблем не наблюдалось — время жизни PR было приемлемым (а это была по сути единственная метрика), а значит все ОК. За нагруженностью ревьюеров просто никто не следил, а качество код-ревью оценить тем более очень сложно.
А стоило всплыть проблеме излишней загрузки ревьюеров «изнутри», и когда стало ясно, что это не случайный всплеск, а закономерность, тогда и начали ее решать, а заодно пересмотрели весь процесс код-ревью и опыт других компаний учли. Не думаю, что это странность.
Вот то что не было нормальных метрик код-ревью раньше — это скорее было ошибкой на мой взгляд, теперь исправились.
А стоило всплыть проблеме излишней загрузки ревьюеров «изнутри», и когда стало ясно, что это не случайный всплеск, а закономерность, тогда и начали ее решать, а заодно пересмотрели весь процесс код-ревью и опыт других компаний учли. Не думаю, что это странность.
Вот то что не было нормальных метрик код-ревью раньше — это скорее было ошибкой на мой взгляд, теперь исправились.
UFO just landed and posted this here
Яндекс ≠ Яндекс.Деньги.
Сомневаюсь, что в Яндексе ещё кто-то пользуется Jira, там есть отличный внутренний багтрекер, рядом с которым Jira несколько лет назад выглядела мамонтом (по скорости работы, интерфейсу, удобству и отказоустойчивости).
Ну и наверняка Император уже навёл порядок и унифицировал код-ревью почти везде.
В code review самый ужас, когда написавший код, является единственным экспертом в области. И это не настолько патетично, как можно себе представить. Например, пишется коннектор из приложения в YASDB. В принципе, фича минорная и YASDB никто использовать всерьёз не хочет, но (другая) компонента использует библиотеку, у которой штатный режим работы — это YASDB. Окей, кто-то пошёл, разобрался как с ней работать. Написал. Засылает PR.
Дальше:
а) Других эспертов в YASDB в компании нет и не планируется.
б) Заславший эксперт только в сравнении с другими (почитал пару дней доки)
в) Логика происходящего понятна только написавшему (т.к. он сопровождает этот кусок).
Разумеется, такой PR принимают, и пользы от такого code review почти никакой. А в codebase прилетает liability в форме куска знаний, которые нужны, чтобы это «трогать».
YASDB = yet another sql database
Дальше:
а) Других эспертов в YASDB в компании нет и не планируется.
б) Заславший эксперт только в сравнении с другими (почитал пару дней доки)
в) Логика происходящего понятна только написавшему (т.к. он сопровождает этот кусок).
Разумеется, такой PR принимают, и пользы от такого code review почти никакой. А в codebase прилетает liability в форме куска знаний, которые нужны, чтобы это «трогать».
YASDB = yet another sql database
UFO just landed and posted this here
Фокус тут не на код. Бывает код, который не хочется втаскивать в компетенцию команды (сделал и забыл). Это плохо для сопровождения кода, но и команда — не резиновая.
Условно, предположим, что это не «sql», а, например, код для работы с библиотекой, которая позволяет считать шаровую часть тензора для передачи в библиотеку, которая по картинке может сказать виляет ли на ней собака хвостом или нет.
Ну напишите вы функциональные тесты. Как будто команда лучше будет понимать что такое шаровая часть тензора и чем оно от девиатора отличается.
Боль же возникает в тот момент, когда это приносят. И не принести нельзя, и ресурсов на разобраться тоже нет.
Условно, предположим, что это не «sql», а, например, код для работы с библиотекой, которая позволяет считать шаровую часть тензора для передачи в библиотеку, которая по картинке может сказать виляет ли на ней собака хвостом или нет.
Ну напишите вы функциональные тесты. Как будто команда лучше будет понимать что такое шаровая часть тензора и чем оно от девиатора отличается.
Боль же возникает в тот момент, когда это приносят. И не принести нельзя, и ресурсов на разобраться тоже нет.
… все это жизнь, а жизнь — это боль. Боль — это хорошо, это показатель, что принесенная часть проекта живая, кем-то используется, и значит — пора выделять ресурсы «на разобраться».
(я так всегда своим коллегам говорю, когда втаскиваем в проект зависимости из разных странных библиотек).
(я так всегда своим коллегам говорю, когда втаскиваем в проект зависимости из разных странных библиотек).
Вы исходите из того, что всё, что делается — это для пользы. Часто бывает так, что что-то делается, и умирает не будучи доделанным. Из-за этого инвестировать силы и время во всё — никакого времени не хватит.
Кстати, как только вы выделили ресурсы на разобраться с одной неведомой фигнёй, у другой неведомой фигни рерсусов стало меньше.
Кстати, как только вы выделили ресурсы на разобраться с одной неведомой фигнёй, у другой неведомой фигни рерсусов стало меньше.
UFO just landed and posted this here
Справедливо. Просто есть большое количество компаний с большой текучкой народа — как айтишного, так и руководящего. И в этом случае, руководящему звену все равно, сколько будет стоить переделка или доделка проекта, сделанного на скорую руку. Так как это будет явно не их головной болью.
Ничуть не возражаю против этого. Но бывает и обратное. Инвестируются гигантские усилия в десятки тысяч долларов (в пересчёте на ЗП) в технологию, под которую бизнес-критикал код постепенно адаптируется, а потом выясняется, что очередной убийца мускуля не совсем ACID, и даже совсем не ACID. Или он ложится под нагрузкой. Или компания решает сменить лицензию и все дружно забивают на её продукт, выпиливают из репозиториев и т.д. После чего, все инвестиции идут лесом. Причём не только в форме потраченных денег, но и упущенного ресурса для тех кусков проекта, которые остались жить.
Оверинвестиции в то, что может и не взлететь — дорого. Недоинвестиции в то, на что будешь полагаться — чревато.
Оверинвестиции в то, что может и не взлететь — дорого. Недоинвестиции в то, на что будешь полагаться — чревато.
Плагин звучит очень здорово, нет ли планов сделать его общедоступным? :) (т.к. проблема близка, идея примерно такая же, только приходится делать это всё ручками)
Мы прямо сейчас как раз делаем плагин для Jira + интеграционный сервис между Jira <-> GitLab, но акцент больше на тикет систему и пайплайны. На вашем опыте вижу, что и ревью тоже классно ложится на эти рельсы. Спасибо за статью!
На загруженность ревьюеров больше всего влияет количество неодобренных PR. Каждый отклоненный реквест — это новая доработка и новое ревью в ближайшем будущем. На мой взгляд, именно здесь наибольший потенциал для оптимизации. Хотя, в Яндекс.Деньгах, возможно, ситуация иная.
Я в свое время, чтобы разгрести завалы code review, составил чек-лист, чего я смотрю в коде. Получилось где-то 100 пунктов. Попросил всех разработчиков перед передачей кода мне на ревью прочекать этот чек-лист и проставить напротив каждого отметку вида Соблюдается/Не применимо/Нарушается там-то по тому-то по согласованию с тем-то.
После этого процесс ревью у меня выглядел так:
Поначалу пришлось повозиться, отсмотреть все замечания по всем ревью за последний год, прочитать комментарии по всем зарегистрированным дефектам, затем скомпилировать это все в чек-лист. Самое сложное было — уговорить команду использовать этот чек-лист. Зато потом трудоемкость code review сократилась в разы, и, самое главное, сократилось количество повторных ревью.
Я в свое время, чтобы разгрести завалы code review, составил чек-лист, чего я смотрю в коде. Получилось где-то 100 пунктов. Попросил всех разработчиков перед передачей кода мне на ревью прочекать этот чек-лист и проставить напротив каждого отметку вида Соблюдается/Не применимо/Нарушается там-то по тому-то по согласованию с тем-то.
После этого процесс ревью у меня выглядел так:
- Чек-лист заполнен? Нет — PR на доработку, минус в карму.
- Смотрю, где в чек-листе отмечены отклонения от требований, и иду туда смотреть
- Если автор имеет низкую карму, тщательно просматриваю весь код.
- Если автор имеет хорошую карму, выборочно просматриваю самые интересные файлы.
- Если нашел косяк, предусмотренный чек-листом, — PR на доработку, минус в карму.
- Если нашел что-то новенькое, тщательно просматриваю весь код, обновляю чек-лист. Создаю задачу на доработку. Если проблема не очень критична, ставлю approve. Если проблема критична, то PR отправляю на доработку.
Поначалу пришлось повозиться, отсмотреть все замечания по всем ревью за последний год, прочитать комментарии по всем зарегистрированным дефектам, затем скомпилировать это все в чек-лист. Самое сложное было — уговорить команду использовать этот чек-лист. Зато потом трудоемкость code review сократилась в разы, и, самое главное, сократилось количество повторных ревью.
Согласно приведённому алгоритму карма в нескольких случаях уменьшается и никогда не увеличивается, т.е. рано или поздно алгоритм сведётся к пункту №3, поскольку низкая карма будет у всех :)
Если автор имеет хорошую карму, выборочно просматриваю самые интересные файлы.
Такого конечно хотелось бы избегать — все-таки мы код в первую очередь ревьюим, а не автора кода.
А что касается чек-листа — это больше похоже на конвенции, например: сборка должна быть «зеленой», должно быть верхнеуровневое описание задачи, форматирование кода отдельным коммитом и тд. Да, у нас тоже есть аналогичные конвенции, и кстати проверка многих из них довольно легко автоматизируется.
UFO just landed and posted this here
Очень интересные метрики и инструмент сбора статистики)
А есть ли у вас данные по ревьюерам? Например, кто больше всего холдит PR-ы.
А есть ли у вас данные по ревьюерам? Например, кто больше всего холдит PR-ы.
Из того, что не вошло в статью, следим еще за долго-живущими PR, то есть за аномалиями: если видим, что PR не вмержен после N дней, то информируем об этом участников PR, а также видим это в статистике.
Но в первую очередь это всегда вопрос к автору PR — именно он больше всех заинтересован в том, чтобы довести свою доработку до production. К ревьюерам у нас обычно вопросов не возникает.
Но в первую очередь это всегда вопрос к автору PR — именно он больше всех заинтересован в том, чтобы довести свою доработку до production. К ревьюерам у нас обычно вопросов не возникает.
Sign up to leave a comment.
Как мы спасали код-ревью