時計を壊せ

駆け出してからそこそこ経ったWebプログラマーの雑記

コードレビューとPull Request、そしてその承認機能の副作用について考える

Programing work

用語

レビュアー
対象となるコードをレビューする人のことを指します。
レビュイー
レビューを受ける人、つまりレビューする対象のコードを書いた人のことを指します。

tl;dr

  • アプリケーション開発業務におけるコードレビューはコードの正しさや質そして一貫性を保ち、それらと同時にコードに対するチームとしての共有知を作り上げる良いプラクティスだと思います
  • アプリケーション開発チーム内でのコードレビューにおいてPull Requestを使ったレビューのスタイルは一般的ですが、Pull Requestの承認は実際にはほとんど意味がないのではないでしょうか？
  • ほとんど意味がないにも関わらず、承認の有無によって業務フローが左右されることでそれが権威的に扱われてしまいオーナーシップを希薄化させ、結果的にコードレビューのコストが増加したりそれを行う目的を見失ってしまっていることはないでしょうか？
  • Pull Requestの承認機能はなるべく利用しないか、利用するにしても既読管理程度の意味合いにとどめたほうがよいのではないでしょうか？
    • 2021/11/08 11:39追記: ただし、これはレビューの必須化をやめるという意味ではありません。承認の必須化によって必ずこういった問題が起きるという主張でもありませんし、例に挙げたような事例はこれが原因だという主張でもありません。原則としてはレビューそのものは必ずやるフローにするべきで、承認必須化の仕組みと承認という言葉がもたらす重みを問題視しています。言い換えるならば、それを上回るメリットがそれぞれのチームにもたらされるかどうかを考えて採用するべきで、思考停止的に有効にするべき機能ではないのではないか？ということです。要はこのあたりも実はよく考えたほうがいいんじゃない？と言いたいだけです。（様々なフィードバックがいただけて大変参考になります。ありがとうございます。）

2021/11/09 00:10追記: どうやら単なる問題の可能性の提起のつもりが、そういった問題がぼくのいるチームで起きているかのような誤解を一部に与えていたようです。そのような事実はありません。(心配させてしまった方にはすみません)
「考える」というタイトルの通りこれは単なる考察です。もちろん、その元となった体験は僕の中にはいくつかありますが、そのなかでジュニアというほどではないはずなのにオーナーシップの希薄化が起きたり権威的なレビューを行う人を見かけることが稀にあり、そのような思考に至る上で何が影響しているのかを考えていて、そのうちの一つとしてこれに至ったという次第です。それがたまたまこのタイミングだっただけで、他意はありません。

前置き

背景を説明するために、まずは一般的なコードレビューのあり方について自分の理解を共有しておきます。

アプリケーション開発業務はほとんどの場合においてチームを組んで行うのが一般的だと思います。 中でも（特にWeb系の企業の）比較的モダンな開発スタイルを取るチームでは、そのコードの変更をチーム内でレビューすることを開発のフローに組み込むことも一般的だと思います。

コードレビューの目的

アプリケーション開発チームがコードレビューを行う目的は様々ありますが、多くの場合は以下のような目的を持ってそれを行うことが多いのではないでしょうか。*1

  • コードの正しさを保つ
    • （たとえば）アプリケーションや関連するAPI・ライブラリの仕様を正しく理解して実装できているか
      • 誤っていそうなコードやコメントはないか
    • （たとえば）その実装方法によって想定外の仕様上の制約を産んでしまっていないか
    • （たとえば）適切なインターフェースをデザインできているか
  • コードの質を保つ
    • （たとえば）パフォーマンス上の問題やセキュリティ上の問題を起こし得る実装になっていないか
      • それは様々な開発上の背景のもと、現状で許容できるレベルを超えうるか
    • （たとえば）適切なアルゴリズムやデータ構造を利用した実装になっているか
    • （たとえば）適切に言語やライブラリの機能を利用できているか
    • （たとえば）コードの追加や変更に対して適切に自動テストの実装や修正が行われているか
  • コードの一貫性を保つ
    • （たとえば）抽象化のスタイルが他と一貫しているか
    • （たとえば）命名規則が適切に適用されているか
    • （たとえば）利用しているライブラリやツールが他と一貫しているか

コードレビューは上記のような目的のもと、様々な観点でコードに対して考察し、それをもとにディスカッションを行います。第三者的な視点でコードを見ることによって、ひとりでは気づきにくかったことに対する気付きを得たり、見落としていたことに気付けたりすることがあるので、その目的に対して大きな効果を上げることになります。（上級者がコードをレビューすることに価値があると考える人もいるかと思いますが、最も重要なことは第三者的な視点が入ることそのものです。）

それぞれの目的にどの程度の比重を置き、またどの程度のコストをかけてコードレビューを行うのかはチーム（あるいは、もしかするとそのメンバーそれぞれで）異なる場合もあるとは思いますがが、おそらくほとんどのチームにおいてはこういったことを目的としてコードレビューを取り入れていることが多いのではないでしょうか。

さらに、コードレビューをチームとして行う過程においてレビュアーとレビュイーがディスカッションを重ねることによって、コードや関連仕様などに対する理解がメンバーの間で深まります。それによって、結果的にチームとしての共有知を作りあげることにもつながっていることもあると思います。
個人的にもそれをいくつかのチームで経験したことがありますがが、これができているチームは高い効率で開発を進められていたように思います。 もっと言えば、個人的にはこれこそがコードレビューを行う最大の目的であるべきではないだろうかとすら思います。

他にも、うまく言語化できないメリットや書きそびれていることも様々あると思いますが、とにかくコードレビューはこういったことを実現する上で、一般的に良いプロダクト作りに有効なプラクティスとして多くのチームで取り入れられているのではないかと思います。
そして、当然ながらコードレビューにはそれにはそれなりのコストが掛かりますが、多くのチームにおいては前述の目的のもとそのコストに見合うメリットを見込んでそれを行っていることが多いでしょう。

コードレビューのスタイル

コードレビューのスタイルは様々ありますが、昨今ではGithubやBitbucket、GitlabなどのPull Request機能を用いてコードレビューが行われることが昨今では多いのではないかと思います。

このスタイルにおいては、まずレビュイーはメインストリームとなるブランチからフォークしたブランチを作成し、そこでコードの変更を行って最低1つのコミットを行い、それをリモートのリポジトリにプッシュしてPull Requestを作成します。

さらに、そのPull Requestに対して必要な説明を（主にテキストを用いて）行い、1名以上のレビュアーにレビューを依頼*2します。

レビュアーによってレビューが行われた後にその変更を承認するかどうかを通知*3し、レビュアーはその結果に応じてそのPull Requestをどうするかを決めます。

承認された場合は、権限を持つ誰かがそのPull Requestをマージします。承認されなかった場合は、レビューされた内容に応じて変更を行ったり、場合によってはPull Requestごと作り直したり、その変更自体をやめることもあります。

もしかすると、細かい部分で異なるということはあるかもしれませんが、多くの場合はPull Requestを利用した開発はこのように行われているかと思います。

前置きが長くなりましたが、今回の話ではアプリケーション開発チームがこのPull Requestを用いた開発フローを採用した上でその過程におけるコードレビューにおいて、Pull Requestとその承認機能がもたらすものに注目していきます。

Pull Requestとその承認機能

Pull Requestをもとにコードをレビューするとき、チーム内でそれを行っている場合においてその承認機能がどのようにチームに作用するのかを考えます。

まず、大前提としてチーム内でコードレビューを行う場合は、レビュアーとレビュイーはその双方がそのプロダクトのコードに対する当事者意識を持って開発しているでしょう。 そのコードがもらたす問題は自分たちの問題であり、その問題は自分たちが自律的に対処するべきであるという意識を持っています。少なくともそうであるべきだと思って開発に臨んでいると思います。

Pull Requestはレビューに限らず、コードの変更をチーム内外に知らせるために作成されます。そしてその歳には前述したような目的に沿ってレビューが行われることが期待されるでしょうし、コードの変更を行ったメンバーは多くの場合はそれを期待してPull Requestを作成しているでしょう。

Pull Requestの承認の意味

では、そのPull Requestを承認したということにはどういった意味があるのでしょうか？

文字通りに読み取ればレビュアーがその変更の取り込みを承認したということですが、実際にはレビュアーだけではなくレビュイーもその変更に限らないコードに対してオーナーシップを持っているはずです。レビュイーはそれが承認されずともその変更を取り込む権限がありますし、もしそのコードが緊急的に即座に取り込まれるべきものであれば取り込むべきです。

つまり、レビュイーは本来であればそのコードが取り込まれることに対して十分な責任とその権限を持っているはずです。 そのため、変更に対するレビューを求める意味はあったとしても、その変更の取り込みの承認を求める意味は特に無いはずなのではないでしょうか？

2021/11/08 12:30追記: コードレビューの文脈外なので触れていませんでしたが、技術監査ないしセキュリティ管理上の何らかの理由で証跡を残したいケースはあるかと思います。ただ、コードレビューの文脈においては、仮にコードを見ていなくても承認を行うことは可能（ノールックLGTMという言葉をご存知でしょうか？）であり、十分にコードがレビューされたことを保証するには不十分です。本当に必要なのは仕組みではなく必ずコードをレビューする必要性の理解とそれを実行し継続するメンタリティであるはずです。

Pull Requestの承認を必須化する副作用

コードレビューはチームとしてコードの正しさや質や一貫性を保つためのセーフティーネットのうちの一つと言っても差し支えないかと思います。 もちろん、それが必ず行われていることは、健全なコードを保つ助けになることでしょう。 そのため、Pull Requestの承認は前述のとおりあまり意味がないとしても、Pull Requestの承認をマージの必須条件にすることで、仕組み的にそれを担保しているチームは多いのではないかと思います。

しかし、そういった意図とは裏腹に、承認という言葉が持つ重みがいくつかの副作用を生んでしまうこともあるのではないでしょうか？

個人的には以下のような副作用が出ることがあるのではないかと考えます:

  • オーナーシップの希薄化
  • レビュアーの権威化

それぞれ順を追って説明します。

オーナーシップの希薄化

Pull Requestの承認をマージの必須条件にすることで、結果的にレビュイーはそのコードがマージできるかどうかの判断をレビュアーに委ねることになってしまいます。 これは見方によっては、レビュアーがその変更の取り込みを承認するまでPull Requestの作成者はその変更をマージできるかどうか判断できないとも捉えることができてしまいます。

そしてその結果として、コードに対するオーナーシップが希薄化してしまうことはないでしょうか？

具体的には、Pull Requestが承認されるまでマージされないことに油断して、安易に変更をPull Requestにしてしまうことはないでしょうか？ *4

レビューの承認機能があることによってそれに甘んじ、あまり推敲せずにそれをPull Requestにしてしまう。つまりコードに対するオーナーシップの意識を承認機能が希薄にさせてしまうことはないでしょうか？

前述した通り、コードレビューにはそれなりにコストが掛かります。経験した人であれば分かると思いますが、前述の目的を達成する上で十分に意味のあるレビューを行おうとすると、コードや仕様やそれらの周辺知識を理解し、それをもとに様々なことを想像して、様々な観点において問題が無いかどうかを考えなければなりません。これは非常にコストが掛かる行為です。

しかし、多くのチームではそれだけ大変なことにも関わらずコードレビューを取り入れています。コードレビューはコードを健全に保つ上で有効な手段ですが、逆に言えばコードを健全に保ち続けることはコードレビューが必要なほど困難であり、それと同時にプロダクトの価値をうまくユーザーに届ける上で重要なことなのです。

安易に変更をPull Requestにしてしまう行為はコードレビューのコストを増加させ、それと同時に健全でないコードがブランチのメインストリームに取り込まれてしまうリスクを増加させます。 前述した通り、コードレビューはコードを健全に保つためのセーフティーネットのような役割なので、そもそもなるべく問題がないコードを書くべきなのは当然のことです。

つまり、本来であればPull Requestの作成そのものにも一定の慎重さを持っているべきで、コードに対するオーナーシップを発揮して万が一レビューをすり抜けても問題がない自信を持ってPull Requestを作成するべきですが、承認という言葉がそのオーナーシップを希薄化させてしまうことで、そういった問題を引き起こす要因の一つになっているなんてことはないでしょうか？

さらに言えば、もし承認を必須化していなくとも、Pull Requestが承認されることに対する安心感によってその変更に対する自信が初めて持てるような状況があったとしたら、それは健全ではない状態でしょう。*5

レビュアーの権威化

繰り返しますが、Pull Requestの承認をマージの必須条件にすることで、結果的にレビュイーはそのコードがマージできるかどうかの判断をレビュアーに委ねることになってしまうことは前述した通りです。 では、それを委ねられたレビュアーはどうなるでしょうか？

レビュアーはある意味でその変更に対してとても強い権限を持つことになります。本来であれば、チームメンバーはそれぞれがそのコードに対して皆オーナーシップを持っているはずですが、 Pull Requestの承認が必須化されている状況下ではレビュアーとレビュイーの間ではレビュアーのほうがより強い権限を持っていることになります。

どういうことなのかイメージしづらいと思うので具体例を考えてみましょう。

たとえば、あるチームのメンバーAがPull Requestを作成し、同じチームのメンバーBにレビューを依頼したとします。 レビュアーであるメンバーBはそのPull Requestをレビューする上で、ある問題に気付きました。レビュイーであるメンバーAはそれでもその問題は些細であり元のコードのままで問題ないと考えていましたが、メンバーBとは意見が合わず、修正を求められました。

結果として、メンバーAはメンバーBのいうとおりに修正を行いました。その変更内容はメンバーBが求めた内容そのもので、メンバーBは問題ないと判断してその変更を承認し、いずれそれはマージされてリリースに至りました。 しかし、その修正箇所が原因で問題が起きてしまいました。それによってこの問題が起こり得ることには、メンバーA/Bの2人とも気付いていませんでした。そして、メンバーAが元々書いていたコードが（たまたま）実際に引き起こす問題が最小限にとどまるようでした。

さて、この場合、コードを書いたのはメンバーAですが、レビュイーであるメンバーAは承認を得るためにはレビュアーの意見に従わざるを得ない、あるいは従ったほうがスムーズに仕事が進むと判断したため、メンバーAはメンバーBの意見に従ったはずです。前述したようなオーナーシップの希薄化もそれに拍車をかけ、メンバーBの意見を鵜呑みにしてしまったのでしょう。

こういった問題は時々みかけることがあるかと思います。いずれも悪気はなく、それぞれがそれぞれの役割を果たそうとした結果として起きてしまった事故です。

しかし、これはレビュアーの権威化が起こっていた結果なのではないでしょうか？ もし承認という概念がなかったならばどうなっていたでしょうか？

承認が必須化されていなければ、そういったコメントは単に意見をおなじ開発メンバーから受け取っただけに過ぎません。 必ずしもレビュアーの意見に従う意味はありませんが、それを無視するためには自分が作成したPull Requestに対してそれ相応の自信と覚悟を持って臨む必要があるでしょう。

しかし、承認が必須化されている場合は、そういったコメントは見方によってはそれに従わなければ（あるいは納得させなければ）その変更を取り入れさせないという脅迫にも映りかねません。 レビュアーが権威化してしまい、健全なコードレビューが行われなくなります。

前述したとおり、コードレビューはコードの変更に対して第三者の視点が入ることそのものがとても重要です。その一方で、コードのオーナーシップを誰もが持って開発に臨むのが健全な開発チームでしょう。 レビュアーの権威化はその第三者の視点が強く影響しすぎてしまいます。

繰り返しますが、コードレビューは本来であればコードの変更に対して第三者の視点が入ることによって、コードを健全に保つことを助けてくれます。 コードを書いた本人が本来であればそのコードを一番理解しているはずですが、レビュアーが権威化してしまっているとそれより強い権限を持った第三者がそのコードをマージさせるかどうかに大きく影響を与えるという状況が生み出されてしまい、このアンバランスな構図はさきほどの具体例のような悲劇を招きかねません。

その場合、コードレビューの目的はどこに行ってしまっているのでしょうか？ レビュアーが権威化していることによって独りよがりなレビューがまかり通ってしまっていないでしょうか？

そしてそういった問題は、実はPull Requestの承認機能がそれを引き起こす遠因となっていないでしょうか？

どうしていったらよいのか

あくまでもこれらの副作用は単なる仮説であり、確実に因果関係があるとまで言えるようなものではありません。単にオーナーシップが希薄なメンバーや、権威的なレビュアーがいるから引き起こされる問題とも考えられるかもしれません。 しかし、Pull Requestの承認機能は前述のとおりあまりメリットがないと思われる割には、こういった副作用を生み出し得る要素を持っているように思えてなりません。つまり、卵か鶏かでいえばPull Requestの承認機能はこの問題の卵のひとつなのではないかというのが自分の意見です。

では、どうしていったらよいのでしょうか？

自分が考える理想的な状況は、メンバーの誰もが健全にオーナーシップを持って開発を行い、マージされても品質を毀損しない自信があるPull Requestだけが作成されることです。*6
そして、それに対してメンバーの誰もが平等な立場のもとでディスカッションを行い、基本的にはテックリードや他のメンバーのサポートのもとにマージの最終的な判断はPull Requestの作成者が行うべきではないかと思います。

その状況を作るために出来ることのひとつは、承認という言葉をやめることです。 既読、程度の意味合いの言葉にとどめるべきでしょう。その程度であればレビュイーは既読が付いただけで安心感を持てるはずもなく、レビュアーが権威化することもなく、同時にコードレビューがコンスタントに行われていることをある程度は保証できるでしょう。
ただし、残念ながらGithubなどでは承認という言葉を変えることはできません。現実的には、チーム内でその意味を読み替えて利用する程度が関の山で、その程度ではその意識が希薄化すればいずれ同じ状況に戻ってしまうでしょう。

もうひとつの方法は、Pull Requestの承認機能を利用しないことです。承認が無い状況ではレビュイーは十分な自信と覚悟がなければそれをマージすることができません。レビュアーが権威化することもなく、フラットにディスカッションが行えるでしょう。
ただし、残念ながらコードレビューがコンスタントに行われていることを保証することはできません。レビューが行われなくともマージはできてしまうので、オーナーシップの持ち方やスキルが未熟なメンバーがいれば結局ルールとしてレビューと一定のメンバーによる承認を必須化せざるを得ないかもしれません。

いずれの方法も銀の弾丸ではありません。しかし、オーナーシップの希薄化やレビュアーの権威化に悩まされている場合は一考の余地があるのではないかと考えています。

また、それと同時に、もし曖昧にコードレビューを行っている場合はその目的意識をはっきり持ち、同時にそれぞれのメンバーがオーナーシップを持って開発を行うことができているかを考えたほうが良いのではないかと思います。

もし、自分自身を内省してコードレビューの目的をうまく認識できていなかったり、安易にPull Requestを出してしまうことがあるとすれば、その姿勢について考え直してみることをおすすめします。

*1:残念ながら、コードレビューの目的をはっきり意識してそれを行うことができているチームは少ないのではないのかと思いますが……。

*2:依頼という行為を行わず、気が向いたメンバーなどでボランティア的にレビューを行うチームもあると思います。

*3:承認機能を利用しないケース、利用しないチームもあると思います。

*4:ただし、実装やPull Requestの作成が素早いことそのものは何の問題もなく、素晴らしいことです。

*5:ただし、これはあくまでもチーム内でそれを行う場合の話であり、チーム外からPull Requestを送って取り込んでもらう場合は例外です。

*6:単なるサンプルや提案を意図したものはこの限りでありませんが、その場合はそれはそのままマージせずに清書して新たにPull Requestを作成することが望ましいでしょう。

ISUCON11予選に参加しました

id:Sixeightとid:aerealとチームにゃんこ選抜というチーム名で予選に参加しました。

再試験スコアは25746点、ベストスコアは記録をちゃんと残せてなかったけど3万ちょっとでした。 予選通過ラインが10万点ちょっとだったので、全然届かずという結果。色々と力及ばず完敗で無念……。

やったこと

事前準備と素振り

ISUCON10の予選問題を使って素振りを行いました。 週末を2日つかって予選突破スコアまで行くところまで体験するという感じのことをやり、チームとしての動き方のイメージをつかみました。

ぼくは主にインフラを中心にアレコレやりつつアプリにも時々手を出すという立ち位置で動くことにしました。

ほか、秘伝のタレ化しているansibleのplaybookを調整したり、alpにpcapサポートを追加するPRを送りつけたりり、チームのScrapboxに計測/調査などのノウハウや勉強ログを共有するためにメモを書き溜めたりしました。 alpのpcapサポートはあまり使ったことのないリバースプロキシなどが出てきたら使おうかなと思っていました。

なお、AWS CDKで一撃で分かりやすいPrivate IPを振ったEIP付きのEC2インスタンスを作って適切なセキュリティーグループを作った上でEIPをRoute53に登録する仕組みを用意していたのですが、 今年の運営は想定外にとても丁寧にCloudFormation Templateを提供してくれたのでこれは徒労に終わりました。（手でRoute53を設定したりしましたが、想定外にSecurity Groupの変更が許されない制約があり、またTLS証明書を事前に用意していなかったのでこのDNSレコードを使う機会は訪れませんでした……）

マニュアルの読み合わせ

アプリの仕様のデカさと複雑さにビビってちゃんと理解してから手を付けようとマニュアルを2時間くらいかけて読み合わせてチームの認識を揃えてました。

最終的に作業時間がもうちょっと取れればこれはなんとかなったのでは……という結果になってしまったり、マニュアルから読み取った最適化のヒントやスコアアップのためのヒントを活かせるところまで辿り着けなかったりと、結果論ですがあまり意味がなかったのでこれは失敗だったかもしれません。バランスの悪い時間の使い方をしてしまったかも……。とはいえ、理解せずに手を付けてもあまり意味がないことも多いので難しい……。

焦らない。ということを念頭に置いた結果、ちゃんと理解して手をつけようということにしていたのですが、予想以上に難しい仕様で時間が思いの外取られてしまったという格好でした。

netdataやalpなどを入れたりなど足回りの整備

ansibleで一発で入れられるようにしておいたので一発でいけました。

ただ、Security Groupで穴をあけるのが許されなかったのでNetdataにアクセスできず、ssh port forwadingもダルいしミスの温床になりそうだしどうしたもんかと困ったところとりあえずこんな感じでルーティングして難を逃れました。

        location /_netdata/isucon11q-1/ {
            access_log off;
            rewrite /_netdata/isucon11q-1/(.*) /$1 break;
            proxy_pass http://127.0.0.1:19999;
        }

        location /_netdata/isucon11q-2/ {
            access_log off;
            rewrite /_netdata/isucon11q-2/(.*) /$1 break;
            proxy_pass http://192.168.0.12:19999;
        }

        location /_netdata/isucon11q-3/ {
            access_log off;
            rewrite /_netdata/isucon11q-3/(.*) /$1 break;
            proxy_pass http://192.168.0.13:19999;
        }

        location /_misc/isucon11q-1/ {
            access_log off;
            rewrite /_misc/isucon11q-1/(.*) /$1 break;
            proxy_pass http://127.0.0.1:29999;
        }

        location /_misc/isucon11q-2/ {
            access_log off;
            rewrite /_misc/isucon11q-2/(.*) /$1 break;
            proxy_pass http://192.168.0.12:29999;
        }

        location /_misc/isucon11q-3/ {
            access_log off;
            rewrite /_misc/isucon11q-3/(.*) /$1 break;
            proxy_pass http://192.168.0.13:29999;
        }

nginxから静的ファイル配信

ぱぱっとやってしまおうと思ったら、ミスや見落としが多かったり思ったようにいかず1,2時間くらい四苦八苦して時間を溶かしてしまいました……。

こんな感じの設定をいれました。

        root /home/isucon/webapp/public;

        location = / {
            rewrite / /index.html break;
            break;
        }

        location /assets/ {
            break;
        }

        location = /register {
            rewrite /.* /index.html break;
            break;
        }

        location /isu/ {
            rewrite /.* /index.html break;
            break;
        }

同時にアクセスログの形式をalpに食わせられるような形式にそろえて、alpでエンドポイント単位の統計情報を作れるようにしました。

MySQL8への変更

dstatでtopを見るにmysqldのCPUがベンチマーク中ずっと張り付いていたので、 初期状態がMariaDBだったのもあり、素振りでも使った比較的慣れているMySQL8に変更することにしました。

ここでまたしても、Ubuntu標準のMySQLをアンインストールしてMySQL Communityのdeb packageからMySQL8をインストールするレシピをうっかりそのまま流してしまい、 MariaDBが入ったままMySQL CommunityのMySQL8をインストールしようとしてインストールの途中で引っかかってpurgeすることもまともにできない状態に陥ってしまい、1時間弱ほど時間を溶かしてしまいました。

アップグレード手順は事前の環境の状態に大きく依存するし、どうせ何度もやる作業ではないので、ansibleでやっつけようとせずに手順書ベースで手でやればよかったと反省しました。

これに伴い、アプリケーションとデータベースを別のインスタンスに分けることに成功し、buffer poolなども調整した設定でMySQLが動かせるようになったことでスコアが少しだけ向上しました。

なお、他の変更も含めてこの時点でスコアが4440点、時刻が14:25ということで残り時間半分を切っていて、それまでミスが続いたのも相まっていよいよマズイと焦りが出てきてしまいました。

インデックスを貼る

GET /api/trend や GET /api/condition/:jia_isu_uuid などで打たれていたクエリがpt-query-digestの上位に上がってきていたので、これを解決できるインデックスを貼りました。

github.com

ただ、これはindexの順番をミスっていて、本来は (jia_isu_uuid, timestamp DESC) に貼るべきでした。焦って確認もミスった……。

他の変更も含めてこの時点でスコアが13332点、時刻が14:57でした。

LIMITを付ける

めちゃくちゃ膨れるisu_conditionからSELECTしてアプリケーション側でLIMITしているやつがあったので、id:Sixeightがcondition_levelを入れてくれたので先回りしてSQLでLIMITするようにしました。

github.com

ログをちゃんと残していないけどまだ他の大きなボトルネックで詰まってたからかスコアには大きく響かなかった気がする。

isu_conditionテーブルへのINSERTバッファリング&直列化

GET /api/trend などでのN+1問題は他のメンバーにまかせていたので、先回りして POST /api/condition/:jia_isu_uuid で行われているisu_conditionテーブルへのINSERTバッファリングと処理の直接化を行いました。

具体的にはこんな感じの関数をGoroutineで立てて、channel経由でinsertするべきレコードをここに送ってまとめた上で複数リクエスト間でまとめてbulk insertすることによって、インデックスの更新負荷やロック獲得負荷を低減させることを狙いました。

func insertIsuConditions(ctx context.Context, logger echo.Logger, ch <-chan insertIsuConditionsArgs) {
    var rows [5000]*insertIsuRow
    var rowsSize int

    ticker := time.NewTicker(50 * time.Millisecond) // 50msごとにflushする
    defer ticker.Stop()
    for {
        select {
        case args := <-ch:
            for _, c := range args.conditions {
                // 呼び出し元でチェックしているのでここではチェックしない
                // if !isValidConditionFormat(cond.Condition) {
                //     return
                // }

                timestamp := time.Unix(c.Timestamp, 0)
                rows[rowsSize] = &insertIsuRow{
                    JIAIsuUUID: args.jiaIsuUUID,
                    Timestamp:  timestamp,
                    IsSitting:  c.IsSitting,
                    Condition:  c.Condition,
                    Message:    c.Message,
                }
                rowsSize++
                if rowsSize == len(rows) {
                    insertIsuConditionDoit(ctx, logger, rows[:])
                    rowsSize = 0
                }
            }

        case <-ticker.C:
            if rowsSize > 0 {
                insertIsuConditionDoit(ctx, logger, rows[:rowsSize])
                rowsSize = 0
            }

        case <-ctx.Done():
            return
        }
    }
}

これに加えてクライアントからは同期的に見えるように sync.Cond を使ってINSERT完了を待ち合わせたりする手なども試したのですが、 待ちが長くなりすぎたのか同時接続数が増えすぎてしまったので断念し、100msを越えない程度に適当なsleepを仕込んでお茶を濁しました。 なお、この同期は動作確認がてらcommitせずにdeployして試してしまったのでログにまともに残っていません。

この変更は他のN+1の修正に先んじてmergeしておいたのですが、GET /api/trend のN+1解消で出てたベンチマーカーエラーの原因特定が間に合わずここでタイムアップを迎えてしまいました。

MySQLのCPU100%張り付きが解消できないままアプリケーションサーバーのリソースがあまり続けてしまって2台構成でフィニッシュです。

感想

ボリュームがとても多く、また解決が難しい問題がたくさんあり、優先順位をちゃんと付けて適切に対処していかないとスコアの上がらないとても良い問題だったと思います。 関係者の皆様、お疲れさまでした！本戦に向けて引き続きがんばってください！

余談

その後、ベンチマーカーを公開してもらえたので、ミスってたところを直したりアレやろうと思っていたことをやってみたりしたところ、ちょっとイジっただけで119022点までいけました。 1時間あればこれくらいは全然できたはずだろうなと思うと悔しい……。

github.com

言い訳

www.youtube.com

実は木曜から金曜にかけての夜から当日まで殆ど眠れないというアクシデントに見舞われており、当日は死にかけみたいな体調でやってました。辛かった……。

なお、ISUCONが終わったあとはさすがに疲労困憊したからなのか、それとも新しい処方に体が慣れてきたからなのか、ぐっすり眠れました。

今夜も眠れるといいなぁ。

Goの並列テストが何並列で実行されるのかを知りたい

Goの並列テストそのもにについては、Mercari Engineer Blogで@yoshiki_shibataさんによって解説されているこの記事が有名かと思います。

engineering.mercari.com

並列に実行されるということは、これはレースコンディション問題に対するテストケースの作成にも利用できるわけです。 sync.Cond などを使わずにできるので便利ですね。

とはいえ、何並列まで同時に実行してくれるのかが分からないと、このような用途ではちょっと困ります。

たとえば、適当な数のテストを並列で走らせるとして、実際の並列実行数を並列テストの数が下回るぶんには正常に実行できますが、それを大きく上回るとデッドロックを起こす場合があります。

そして、実際の並列実行数は最初に貼った記事のとおり-parallelオプションかあるいはそのデフォルト値であるGOMAXPROCSによるので、すなわち環境依存で失敗しえるテストになってしまいます。 環境依存で失敗しえると本当の失敗を見逃しやすくなってしまうのでよろしくありません。

これを解決するためには、小さな数に並列テスト数を固定してしまう手もありますが、あまり小さな並列数に固定してしまってはレースコンディションが起きそうな状況を引き起こせる確率が低く、また十分に低くなければ前述のように実行環境依存でらデッドロックを引き起こしてしまいかねません。

かといって並列実行数を得られるインターフェイスは提供されていなさそうです。

じゃあ無理やり取るかというわけで暴力です。暴力は全てを解決する……。

func mustGetParallelCount() int {
    tp := flag.CommandLine.Lookup("test.parallel").Value.String()

    parallel, err := strconv.Atoi(tp)
    if err != nil {
        panic(err)
    }

    return parallel
}

こんな感じで取れます。flagはCommandLineという変数にグローバルなコマンドライン引数の処理結果を持っているのでそこからLookupすれば取れるという寸法です。

これを使って並列テスト数を調整したり、場合によってはテストケースごとSkipするなどすればよさそうです。めでたしめでたし。

とはいえ、若干筋悪っぽい感じもするのもっとで良いアイデアあったら教えて下さい。

WEB+DB Press Vol.119のPerl Hackers Hubに寄稿しました

perl

WEB+DB PRESS vol.119の表紙
WEB+DB PRESS vol.119

Perl Hackers Hubも第64回ということでキリが良いですね。 個人的にはありがたいことに3度目のPerl Hackers Hub掲載です。

今回は「少しマニアックなPerlのテクニック」ということでPerlにまつわる少しニッチなTips集のようなものを書かせていただきました。

特殊変数を使って短くシンプルにコードを書き上げるテクニックであったり、Perl組み込み関数のsyscallを使って任意のシステムコールを呼び出す方法などを紹介します。 もっとPerlを使いこなしたい！と思っている方へのヒントとなるような内容を届けられたらと思っております。 もちろん、CPANモジュールを使わないことを推奨するわけではなく、あくまでもPerl本体の機能だけでもここまでのことができるぞという紹介になっております。

Dockerコンテナに潜ってDockerfileのデバッグをしたり、FaaSでprintデバッグをしたりすることも多い昨今ですが、まだまだオンプレミスの環境やその流れで構築されたIaaSの環境も多いことでしょう。 そのような環境でいざというときにログやデータを調査するための道具が少し足りない！というときに、CPANモジュールなどの外部パッケージのインストールが気軽にできる環境というのはそう多くはありません。

そのような環境で、外部パッケージに頼らず、多くの環境に最初からインストールされているPerlそのものの機能を使いこなすことができると、その際の解決策の選択肢の幅が広がります。 Perlそのものの機能も使いこなせれば、CPANモジュールなどの外部パッケージのインストールをする必要もなく、より素早く問題を解決できる場面もあることでしょう。 その際に役に立ったり、あるいはそのヒントになるようなものを提供出来たらと思っています。

そういうわけで、今回のテーマを書かせていただきました。 明日10/24に発売となりますので、もし良ければ書店等でお買い上げいただき、読んで頂けますと幸いです。

WEB+DB PRESS Vol.119

WEB+DB PRESS Vol.119

編集を担当して頂いた技術評論社の稲尾さんをはじめ、直接監修頂いた牧さん、ほか勤務先など各方面の関係者の方々にも様々な面で協力してもらい助けて頂きました。 この場を借りて改めて御礼申し上げます。ありがとうございました。

TypeScriptでタプル型の順列を得たい

TypeScript

たとえば、 [1, 2, 3] というタプル型があった場合に [1, 2, 3] | [1, 3, 2] | [2, 1, 3] | [2, 3, 1] | [3, 1, 2] | [3, 2, 1] みたいな組み合わせが欲しい。 これは順序を指定するようなケースに型制約を持たせるときに役立ち、io-tsなどを使ってType Guard関数を生成すれば外部入力に対しても型エラーが捕捉できる。

サッとググった感じ、自分のググりパワーが足りないのかうまいソリューションがみつからなかったので、あーでもないこーでもないとやって諦めてベタッと書いたところこんな感じになった。

type Permutations2<T extends readonly any[]> = [T[0], T[1]] | [T[1], T[0]];
type Permutations3<T extends readonly any[]> =
  | [T[0], ...Permutations2<[T[1], T[2]]>]
  | [T[1], ...Permutations2<[T[0], T[2]]>]
  | [T[2], ...Permutations2<[T[0], T[1]]>];
type Permutations4<T extends readonly any[]> =
  | [T[0], ...Permutations3<[T[1], T[2], T[3]]>]
  | [T[1], ...Permutations3<[T[0], T[2], T[3]]>]
  | [T[2], ...Permutations3<[T[0], T[1], T[3]]>]
  | [T[3], ...Permutations3<[T[0], T[1], T[2]]>];
type Permutations<T extends readonly any[]> = {
  2: Permutations2<T>;
  3: Permutations3<T>;
  4: Permutations4<T>;
}[T["length"] extends  2 | 3 | 4 ? T["length"] : never];

もっとうまくできそうな気がするが無限再起と判定されたり難しかった。n行ぶん記述すれば良いので雪だるま式に記述量が増えることもないが、添字を書き間違えたら変なタプル型が混じることになるのは微妙っぽい。 無限再起を回避する方法はboost-tsの実装とか参考にしたけど定数ぶんまでのパターンしかサポートしないということにするしかないのだろうか。

もっとうまく書けるぜ！ってひといたら教えて下さい。

これだった（教えてもらった）:

susisu.hatenablog.com

やっぱ素直に再帰すると無限再帰扱いでエラーになるんだなぁ。 ベタに書くのも悪くないだろうか。

こういうのもあると教えてもらった:

github.com

シンプルでよさそうだけど無限再帰エラーにかからないのはobject型（っていうのかこれ？）をつかっているからだろうか？