こんにちは。メルカリのBackendエンジニアの@osari.kです。
この記事は、Mercari Advent Calendar 2023 の9日目の記事です。
一般に大きなプルリクエストはレビューが大変で、マージまでに時間がかかります。一方で複数の小さいプルリクエストに分割するとコードレビュー待ちの間、関連する開発がブロックされることがあります。今回は機能の開発時間を短くするために、チームで試したGitのブランチ戦略の1つであるStacking手法をケーススタディを交えて紹介します。
大きなプルリクエストがもたらす問題点
大きなプルリクエストがもたらす問題とは何でしょうか?
- コードレビューで読むサイズが増える
- コードレビュー中の修正回数が増える(可能性が増える)
- コードレビューで必要な知識の範囲が広がる(可能性が増える)
- 変更箇所が多いのでリリースのリスクが増加する
プルリクエストが大きいということは、含まれる変更箇所が多いということです。それはつまり、コードレビューで読むサイズが増え、レビューの観点も増え、レビューにあわせて行われる修正も多くなるでしょう。
モノリシックなレポジトリの場合コード全体に精通しているエンジニアは少なく、一般に複数のドメインにまたがる変更の場合、複数のチームからレビューの承認を貰わなければなりません。
大きな変更を一度にリリースすると、障害が発生する可能性が増加し、障害が発生したときに原因の特定も難しくなります。
一方で、関連する変更を複数の小さいプルリクエストに分割すると、コードレビューの間関連する次の作業が進められないという課題もあります。
Stacking手法による小さいプルリクエストの推進
本記事ではこれまでに述べた問題を改善するために私達のチームが試してみたStacking手法について得られた知見を共有したいと思います。
Stacking手法は以下のBlogで紹介されています。
Stacked Diffs (and why you should know about them)
関連する部分を要約すると、Stacking手法は、1つの大きな変更を分割して管理できるようにするプロセスです。ある変更が別の変更に依存している場合、それらは変更差分のスタックとして組み立てられ、正確な順序でマージされます。プルリクエストに対してさらに機能を追加するプルリクエストを作る様をスタックと例えています。これにより、変更をレビューしやすくなります。
合わせて、Stacking手法は複数の作業を並行して行うのに役立つという利点もあります。各段階で作業を終了させることができるため、時間を節約できます。以上のように、Stacking手法はコードの変更の複雑さを管理するための強力な手段であり、上手く実装した場合、生産性と効率性を高めることができます。
本来は上記のBlogで紹介されているスタック間の差分を見やすくするツール(ReviewStack など)と組み合わせて使うのがいいと思います。
この手法はチームメンバーがSlackで共有してくれて、一度チームで試してみようということでGitHubのプルリクエスト機能のみを使い試しました。
Stacking手法の目的
なぜこの手法を試すのか、それはコードレビューで開発者の他の開発をブロックしないためです。
例えばある機能を実装するときにそれを部分タスク PR1,PR2,PR3に分割し、PR2がPR1に依存、PR3がPR2に依存といった依存関係があるとしましょう。
図1: プルリクエストの依存関係
通常のプルリクエストの作り方の場合、PR1のマージが終わるまでPR2, PR3の開発はブロックされます。
※実際には開発者はローカルでの開発は可能ですし、PR1がマージされる前にPR2のコードレビューを依頼することもできます。GitHubのプルリクエスト機能のみで試す場合に重要なのはチームで認識を揃えることだと思います。Stacking手法を使うことを共有しておくことで、混乱なくコードレビューがスムーズになります。
この方法は1人の開発者が自分のプルリクエストに依存した開発をコードレビューにブロックされずに行うためのものなので、複数人で関連機能を開発する場合には向きません。
というのも、Stacking手法は定期的にRebaseが発生します。自分だけなら、Rebaseの影響範囲や、影響が出るタイミングをコントロールできますが、複数人の場合頻繁なRebaseは開発効率を落とします。
モノリシックなレポジトリに実装されているマーケティングシステムの改善での実例
今回Stacking手法を試した一連の機能変更について紹介します。
今回はマーケティングシステムのスケーラビリティ改善に対してStacking手法を適用しました。今回改善を行うマーケティングシステムはモノリシックなレポジトリに実装されています。
ディスカウントを適用する前に商品のいくつかのデータをチェックします。そのうちの一つは別のマイクロサービス(Validation MS)の持つデータを用います。
Validation MSへのクライアント機能、その結果を使いやすいように変換する関数がモノリシックなレポジトリに実装されています。
MSごとに別々のチームがメンテナンスしており、意図した変更を達成するにはマーケティングシステムとは異なるValidation MSのドメイン知識が必要になります。
マーケティングシステムは定期的に実行されるCronjobとWorkerがQueueで接続されています。処理すべきデータは複数あり、Cronjobから処理すべき商品のIDをQueueに送り、Workerで個別にValidation MSのAPIを利用して処理を行います(図2)。
図2:改善前のシステム構成
Validation MSのAPIはBatch呼び出しもサポートしているので、Cronjob側でBatch APIを用いて事前処理をし、Worker側ではValidation MSへのアクセスをしない形にすることでスケーラビリティの改善を試みました(図3)。
図3: 改善後のシステム構成
簡易検証を行った後に、プルリクエストを作成しました。
Stacking手法で作成したプルリクエストは6つです
- 不要な変数の削除
- 不要なロジックの削除
- 不要なメソッドの削除
- Validation MS関連機能のユニットテストの改善
- Validation MSのBatch エンドポイントの結果変換メソッドの修正
- WorkerからCronjobにValidatio MSの呼び出しとチェック機能の移動
プルリクエスト1~3では4以降で安全に変更するために、不要なコードを削除をして全体の見通しを良くしています。不要な変数を削除(プルリクエスト1)すると、不要なロジックが削除(プルリクエスト2)できて、不要なメソッドの削除(プルリクエスト3)に辿り着くという依存関係があります。
プルリクエスト4と5はValidation MSの開発チームと協力して実装・コードレビューをしてもらいました。メソッドのシグネチャが定まれば、マーケティングシステムチーム内で並行してプルリクエスト6を実装・コードレビューをしてもらうことができます。
図4: Stacking手法を用いた場合の開発フロー
上述のように各プルリクエストは自分より小さい番号のプルリクエストに依存しています。
そのため、Stacking手法でない場合、開発者は毎回コードレビューでブロックされてしまいます。
Stacking手法を用いた開発タイムライン
Stacking手法で実際に開発がどうなったかをイメージしやすいように、今回の事例について一連の変更のデータをGitHubとSlackから取得しタイムラインとしてまとめました(図5)。
開発フェーズを以下の3つに分類しました
- 準備:コーディング前に情報収集や実装方針を議論する期間
- コーディング:コードを書き始めてからレビューを依頼するまでの期間
- 今回は最初のCommitをコーディング開始時刻としました。
- コードレビュー:Slackでコードレビューを依頼してからGitHub上でApproveされるまでの期間
図5の開発タイムラインのように、開発がコードレビューにブロックされていないことと、並行してプルリクエストのレビューをしてもらった様子がわかります。
今回の開発ではPoCにより大枠の方針を定めた後に実際の開発を進めており、結果として各プルリクエストのコーディングフェーズが短くなっています。
図5: Stacking手法を用いた開発タイムライン
Stacking手法の評価
この一連の変更でStacking手法を使ったことで、得られたメリットは以下になります。
1: プルリクエストのサイズを小さく保てた
評価のために、プルリクエストを分割しなかった場合のプルリクエストサイズとStacking手法の各プルリクエストのサイズを比較してみます(表1)。ここでは以下の定義で比較します
- Diff: 追加行数と削除行数
- プルリクエストサイズ: 追加行数と削除行数の合計値
比較すると、一番大きなPR4で40%のサイズになっており、他は13%〜22%のサイズになりました。
Stacking手法ではプルリクエストのレビューに後続の開発がブロックされないため、待ち時間の増加を気にせず、適切な粒度でプルリクエストを作成できました。
特に顕著なのがプルリクエスト1,2,3の分割だと思います。不要なコードの削除を1つのプルリクエストにまとめず、レビュアーにも理解がしやすい形で作成できました。
また、分割した各プルリクエストサイズの合計についても分割しなかった場合と比較して102%のサイズで、わずかに増加していますが分割のメリットを考慮すると許容範囲でしょう。
プルリクエスト | Diff追加行数 | Diff削除行数 | プルリクエストサイズ | 分割無しと比較したプルリクエストサイズ |
---|---|---|---|---|
分割無し | +615 | -837 | 1452 | — |
PR1 | +2 | -5 | 7 | 0.5% |
PR2 | +48 | -268 | 316 | 22% |
PR3 | +0 | -186 | 186 | 13% |
PR4 | +318 | -269 | 587 | 40% |
PR5 | +114 | -72 | 186 | 13% |
PR6 | +149 | -53 | 202 | 14% |
PR1〜6の合計 | +631 | -853 | 1484 | 102% |
2: レビュー依頼開始からApproveをもらうまでの時間を短く保てた
Validation MSのBatch エンドポイントの結果変換メソッドの修正を行うPR5はValidation MSのドメイン知識が必要だったため、今回はSlack上で大枠の説明をしてもらったあとに、プルリクエストレビューを通じてドメイン知識の獲得を行う方法で進めました。そのためPR5のレビューには時間がかかっています。
それ以外のプルリクエストはレビュー依頼の翌日にはレビューが完了してマージできたため、効率よく開発を進められました。
3: コードレビュー中に開発を並行して進めることができた
図5の開発タイムラインを見るとレビュー中に最初のcommitを行っていることがわかります。
実際には最初のCommit前から開発をしているため、コードレビューにブロックされることなく後続の開発を進められていることが確認できます。
コーディングと同様にコードレビューも並行して行われていることがわかります。
通常の分割手法を行った場合はレビューが完了するまで後続の開発を行わないため、コードレビューも並列で行われません。そこで、各プルリクエストのレビュー時間の合計を通常の分割手法の場合のレビュー時間と仮定します。
Stacking手法によるレビュー時間をPR1のレビュー依頼からPR6のApproveとして比較すると、Stacking手法によりレビュー時間を7%削減できたことになります。
実際には並列でコードレビューを行わないことで、各プルリクエストのコードレビュー時間が短くなる可能性もありますが、プルリクエストはGitHub上のコメントによる非同期なコミュニケーションが主であることから、相手からの返信を待つ時間が支配的となるため複数のプルリクエストを並列で行うメリットは大きいと考えられます。
4: コードレビューを専門性のある別々のチームに依頼できる
Validation MSドメインにフォーカスした小さいプルリクエスト(PR4, PR5)はマーケティングシステムの詳細を知らないValidation MSチームがレビューしやすくなっています。結果としてValidation MSチームの複数名の開発者がレビューに参加してくれて、コードの質が向上しました。
具体的には、当初はマーケティングシステムに実装されていたValidation MSのSingle IDエンドポイントの変換メソッドとBatchエンドポイントの変換メソッドの詳細が異なっており、マーケティングシステムの挙動を変えないために、Single IDエンドポイントと同じ変換を行う新規Batchエンドポイントの変換メソッドの作成を考えていました。それがValidation MSチームとSlack上で議論していく中で、この実装の詳細の差分は、既存のBatchエンドポイントの変換メソッドの修正を行うことで安全に解消できました。
5: 小さいプルリクエストは開発者自身も思い出しやすい
プルリクエストのサイズが小さくなることで、レビュアーがレビューしやすくなるだけではなく、開発者自身にもメリットがありました。マーケティングシステム開発の他に別の機能の開発や、メンバーのコードレビュー、ミーティングへ参加をしていると、開発した際の記憶が薄れていて、レビューコメントに対応するために再度思い出す作業が必要になります。プルリクエストが小さいことで思い出しやすく、コンテキストスイッチが多い中でもレビューコメントに対応しやすかったです。またメリット2で述べたレビュー時間が短くなったこともあり記憶が薄れずに対応できた割合も多かったです。
結論
本記事ではGitのブランチ戦略の1つであるStacking手法をチームで試した結果を整理しました。レビュー待ちの課題を解消することで、プルリクエストの分割を推進し各プルリクエストのサイズを13%〜40%と小さくすることができました。また並行して開発し、レビュー依頼をすることでレビュー時間を7%削減できました。このようにコードレビューにブロックされることなく開発ができるメリットを得ることができました。
機能開発をする中でコードベースへの理解が深まり、小さい改善点を見つけることがよくあります。そうした場合に、改善を後回しにせず、小さいプルリクエストとして分割してレビュー依頼できる点も開発者体験が上がったと感じました。
また想定外の恩恵として専門ドメインで閉じたプルリクエストを作り、専門チームにレビューをしてもらうことで、コードレビューでフォーカスする点が明確になり、最終的なコードの質が上がりました。この点はStacking手法を使わない場合でも継続して意識していきたいと思います。
明日の記事は reyさんです。引き続きお楽しみください。