Reviewing a Patch/ru

From PostgreSQL wiki

Jump to: navigation, search

Многим кажется, что у них маловато квалификации для полной проверки патчей PostgreSQL. Однако проверка состоит из множества шагов. При этом вы можете помочь проекту на ранних стадиях проверки. Если вы умеете накатывать патчи и использовать новые фичи, значит, вы достаточно подготовлены для их ревью.

Коммитеры также проверяют патчи перед вливанием в базовый код. Перед проверяющими стоит задача максимально снять нагрузку с коммитеров, обнаруживая и устраняя простые проблемы. Проверяющий не несет обязательств по качеству, а лишь отчитывается о найденных проблемах. Этот шаг считается выполненным, как только проверяющий сочтет, что патч готов для более глубокой проверки коммитерами. В качестве примера обратитесь к этому ревью. Проверки разных патчей, конечно, могут отличаться друг от друга, все зависит от конкретной задачи.

Обратите внимание на насмешливую презентацию Джошуа Толле (Josh Tolley), и более серьезный Обзор процесса проверки патчей.

Важные моменты:

  • commitfest, взятые в работу, доступны здесь.
  • У каждого commitfest есть менеджер, которого можно попросить о помощи или задать вопросы организационного характера.
  • Согласование на проверку не требуется.
  • Пожалуйста, подписывайтесь на проверку, как запланируете ревью. Некоторые участники снимают заявку до фактического завершения проверки. Так делать не стоит, это усложняет планирование.
  • С другой стороны, мы просим, чтобы первоначальная проверка укладывалась в 5 дней, чтобы избегать блокировок на этой стадии. При этом возможно провести частичную проверку, либо запросить больше времени. Достаточно оставаться на связи.
  • Основное общение по вопросам ревью и разработки должно производиться через рассылку pgsql-hackers.
  • Результаты проверки необходимо отправлять ответом на письмо, содержащее исходный патч. Соблюдайте целостность переписки, иначе автор патча может не увидеть результатов проверки.

Типичные стадии проверки:

Contents

Подача заявки на проверку (требуемые навыки: патч, понимание английского языка)

  • Патч предоставлен в 'context'-формате с контекстом? (см. context-формат diff)
    • Патчи diff в normal и plain-форматах, в которых отражены только строки без контекста не приемлемы.
    • По хорошему, в подаваемой заявке патч должен быть в context-формате (diff -c) или unified-формате (diff -u), что значительно облегчает чтение кода.
  • Ложится ли патч на master-ветку git без проблем?
  • Содержит ли патч необходимые тесты, исправления документации и тд.?

Проверка удобства использования (требуемые навыки: статическое тестирование, умение находить и читать спецификации)

Прочтите описание назначения патча и обдумайте:

  • Действительно ли патч реализует заявленное?
  • Нужно ли это вообще?
  • Есть ли это на текущий момент?
  • Соблюдены ли SQL-спецификации, или поведение, принятое сообществом?
  • Включает ли поддержку pg_dump (если применимо)?
  • Несет ли угрозы?
  • Все ли обосновано?

Функциональное тестирование (требуемые навыки: использование команд patch, configure и make, вывод ошибок в лог)

Накатите патч, скомпилируйте и протестируйте:

  • Работает ли функциональность, как заявлено?
  • Существуют ли частности, неучтенные автором?
  • Выводятся ли предупреждения или падает код?
    • Ревью необходимо проводить со включенными --enable-cassert и --enable-debug флагами configure, см. Работа с git. Это позволит обнаружить возможные пропущенные проблемы. Стоит помнить, что собранный с этими флагами PostgreSQL может работать значительно медленнее, чем без этих флагов. Если проверяется патч, связанный с производительностью, то непосредственно перед тестом на скорость, необходимо пересобрать уже без этих флагов. Некоторые издержки флага --enable-cassert можно снизить за счет опции debug_assertions = false конфигурационного файла postgresql.conf. Более подробно см. Опции для разработки. По умолчанию эта опция включена, если сервер собран с флагом --enable-cassert. Также использование флага --enable-debug однозначно приведет к издержкам с применением большинства компиляторов, кроме некоторых, таких как gcc, например.

Проверка производительности (требуемые навыки: умение замерять производительность)

  • Приводит ли патч к снижению производительности на простых тестах?
  • Заявлено ли улучшение производительности в описании?
  • Не затрагивается ли производительность других компонентов?

Проверка кода (требуемые навыки: оценка согласно принятому стилю, опыт с переносимым кодом, базовое умение чтения кодов на C)

Прочтите изменения кода и задумайтесь:

  • Соблюдается ли принятый стиль?
  • Есть ли проблемы переносимости?
  • Заработает ли на Windows/BSD и тд.?
  • Есть ли комментарии и в достаточном ли объеме?
  • Выполняется ли заявленная логика и корректно ли?
  • Генерируются ли предупреждения при компиляции?
  • Можете ли вы свалить патч?

Проверка архитектуры (требуемые навыки: опыт в архитектуре PostgreSQL)

Задумайтесь о вносимых изменениях в контексте всего проекта:

  • Все ли согласуется в рамках проекта, функциональности/модули?
  • Вносятся ли взаимозависимости, которые могут привнести проблемы?

Оценка проведенной проверки

  • Проведена ли полная проверка всех предполагаемых моментов со стороны проверяющего?
Personal tools