時計を壊せ

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

ISUCON12予選に1人で挑み惨敗を喫しました

FAILでスコア0でフィニッシュです。ありがとうございました……。

経緯

近頃、チームで仕事するの難しいなーと感じていたところ、気が狂い、いっそのこと俺一人で全部やりてーとなって、血迷いました。難しさから逃げるな。

やったこと

最初に動いているコードはGoでした。

Goのまま行ってもよかったのですが、せっかく好きな言語であるところのPerlの初期実装が用意されているわけだし、Perlを使うことでチームメイトが手を出しづらくなるわけでもないし、RDBを使った開発経験*1としては一番長いPerlで挑もうということでPerlに切り替えます。 なにより、Perlで勝てたらカッコいいじゃんと思っていました。負けたが……。

兎にも角にも、まず計測。

ベンチを回し、事前に用意したAnsibleによってセットアップしたNetdataでOSリソースの消費バランスを見つつ、アクセスログをalpで集計しました。 ISUCON11予選と同様にセキュリティグループの変更は制限されていそうだったので、こんな具合でnginx経由でNetdataにアクセスできるようにしておきます。

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

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

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

https://github.com/karupanerura/isucon12-qualifier/blob/main/ansible/files/config/nginx.conf#L113-L129

オープニングムービーの通り、ランキングを提供するエンドポイントが圧倒的な処理時間を占めています。また、I/O waitが大変に高い値を示していることが分かりました。 (本当はtopも見る手筈だったけどうっかり抜けていた。。)

では、と、実際にコードを読み始めてみるのですが、SQLiteだったので少々面食らいます。コード量も多く、焦りを感じます。

コード量も多く一人なのもあってつい焦ってしまい、I/O waitの原因は都度openしているSQLiteを通じた、ディスクへのアクセス過多だろうと早合点してしまいました。 本来はちゃんと計測して切り分けをするべきです。

スキーマを見ると明らかにIndexが効いていないのでとりあえずIndexを貼ってベンチを流したところ、ほとんどスコアが変わりません。

実は、このとき、実はスキーマの定義ファイルだけを変更しただけで初期データに対してIndexを貼るのを忘れており、またしても早合点していました。 これでは新しく作られたテナントに対してしかIndexが効かず、実際にはID=1のテナントがかなりヘビーに使われていたため、ほとんど効果がないのは当然です。 この結果をすぐに疑うべきでしたが、ここでSQLiteのままチューニングすることに対する懐疑心が強くなります。

また、SQLiteのままでは、普段使っているMySQL向けのツールが使えません。 Profilerも用意されていましたが、初見のプロファイリングのログをうまく集計/解析して頑張ってSQLiteのままチューニングしきれる自信はありませんでした。

さらに、SQLiteからMySQLへの移行を補助する簡易なスクリプトも用意されているのを発見したので、MySQLへ頑張って移行するぞという気持ちが強くなります。

そして、MySQLへの移行を決意しました。すでに判断ミスが多い……。

テナントDBのMySQL移行

SQLiteのDBをMySQLに移行するにあたって、それぞれを別のデータベースとして作ってしまうのがよかろうという発想に自然となりました。データベース名は isucon_tenant_%d のようにしました。

そして、IDは数値なので、IDの2の余剰で2台に分散させることができそうです。実際にシャーディングするかはさておき、ベンチマーカーで負荷が高くなっていった際に新しくヘビーに使われるテナントが発生するシナリオはありえなくはなさそうですし、そういったテナントを隔離するためにもDBを選択できる実装にしておくのはよさそうな気がしたのでついでにやってしまうことにしました。

幸い、SQLiteのファイルがテナント毎に別れていためにそのようなシャーディングの実装は簡単にできそうでした。

どうせMySQLに移行するなら同時にスキーマも最適化してしまうことにします。 幸い、今回は1人チームなので他のひとの作業をブロックすることはありません。

SQLiteからMySQLへの移行はそこそこ時間がかかりそうだったので、シャーディングすることを考えたときに同居する可能性があるAdmin DBをどうするか見ていきます。

Admin DBのRedis移行

Admin DBをどうするかを考えるためにスキーマと実装を追ってみると、 id_generatorvisit_history がつらそうなことに気づきます。

いずれもハードに利用されるテーブルであり、特に visit_history はランキングを提供するエンドポイントにアクセスする度にINSERTされているのでボトルネックになりやすいかなり邪魔げです。しかし、実際には visit_history は.finished_at 以前の最終アクセス日時しか求められていないようでした。ランキングを提供するエンドポイントで finished_at 以前の期間だけ更新するようにすればRedisのSET型で十分そうです。これもついでにやってしまうことにしました。

id_generator もRedisでINCR/INCRBYを使って代替できそうです。UUIDやULIDに変えることも考えましたが、現状のID発番は連番となっておりそれをわざわざ16進数に変換して扱っていたため、連番であることや外形的に16進数表記であることに意味がある可能性があると考えてそれはやめておきました。なお、いずれも問題なかったそうです。残念。

これをやりきれば、Admin DBはテナントの定義だけになり、かなりシンプルになりそうです。負荷はほとんどなくせるでしょう。

ついでにガッと移行してしまうことにします。

テナントDBの最適化

SQLiteからMySQLへの移行がなかなか終わらないので、テナントDBの最適化も先回りして考え始めます。

ランキング情報を作るための player_score はCSV経由でインポートされます。 flockでロックを取ってアイソレーションが行われていますが、DELETEする際にtenant_idcompetition_idで絞り込む際に適切にロックを取れるはずです。トランザクションにまとめることでこのflockは外せるでしょう。

また、N+1クエリとなっているクエリを読み解くと、各player_id毎に最後の行のスコアを採用してそれをscore順に並び替えていることがわかります。 他の箇所でも最後の行以外は要求されていなさそうでした。CSVからインポートする際に各ユーザーごとに最後の行だけを採用すればよさそうです。(後述しますが、この考えは実際には少し間違っていました。)

ここで、初期データを修正する必要性に気付きますが、修正は窓関数を使えばSQL一発で出来るので他のALTERと一緒にやってしまえばよさそうです。(このときに、先のINDEXの適用が実はできてなかったと気づければ引き返せたのだけど、気付かず。)

SELECT
   id, tenant_id, competition_id, player_id, score, row_num, created_at, updated_at
FROM (
    SELECT
        id, tenant_id, competition_id, player_id, score, row_num, created_at, updated_at,
        ROW_NUMBER() OVER (PARTITION BY tenant_id, competition_id, player_id ORDER BY row_num DESC) AS `rank`
    FROM player_score) a
WHERE a.rank = 1;

実際には、idは使っている箇所がないのでDROPできますし、row_num も使うことはないので残す必要はありません。(後述しますが、この考えは実際には少し間違っていました。)

さらに、IDが16進数表記の文字列になっているわけで、単調増加なぶんB+Treeの挿入効率は悪くはないのですが、無駄にインデックスサイズが大きいのが気になります。 (ボトルネックではないので)放っておけばいいものの、ついでなのでbigintにすることにしました。変換はMySQLのCONV関数で一発です。

……などを織り込んだものがこちらです:

-- competition
CREATE TABLE `competition_new` (
  `id` bigint NOT NULL,
  `tenant_id` bigint NOT NULL,
  `title` text NOT NULL,
  `finished_at` bigint DEFAULT NULL,
  `created_at` bigint NOT NULL,
  `updated_at` bigint NOT NULL,
  PRIMARY KEY (`id`)
) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_0900_ai_ci;
INSERT INTO competition_new (id, tenant_id, title, finished_at, created_at, updated_at) SELECT CONV(id,16,10) AS id, tenant_id, title, finished_at, created_at, updated_at FROM competition;
RENAME TABLE competition TO competition_old, competition_new TO competition;
DROP TABLE competition_old;

-- player
CREATE TABLE `player_new` (
  `id` bigint NOT NULL,
  `tenant_id` bigint NOT NULL,
  `display_name` text NOT NULL,
  `is_disqualified` tinyint(1) NOT NULL,
  `created_at` bigint NOT NULL,
  `updated_at` bigint NOT NULL,
  PRIMARY KEY (`id`)
) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_0900_ai_ci;
INSERT INTO player_new (id, tenant_id, display_name, is_disqualified, created_at, updated_at) SELECT CONV(id,16,10) AS id, tenant_id, display_name, is_disqualified, created_at, updated_at FROM player;
RENAME TABLE player TO player_old, player_new TO player;
DROP TABLE player_old;

--- player_score

CREATE TABLE `player_score_new` (
  `tenant_id` bigint NOT NULL,
  `player_id` bigint NOT NULL,
  `competition_id` bigint NOT NULL,
  `score` bigint NOT NULL,
  `created_at` bigint NOT NULL,
  `updated_at` bigint NOT NULL,
  PRIMARY KEY (`tenant_id`, `player_id`, `competition_id`),
  INDEX `ranking_idx` (`tenant_id`, `competition_id`, `score`)
) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_0900_ai_ci;
INSERT INTO player_score_new (tenant_id, competition_id, player_id, score, created_at, updated_at) SELECT tenant_id, CONV(competition_id,16,10) AS competition_id, CONV(player_id,16,10) AS player_id, score, created_at, updated_at FROM (SELECT id, tenant_id, competition_id, player_id, score, created_at, updated_at, ROW_NUMBER() OVER (PARTITION BY tenant_id, competition_id, player_id ORDER BY row_num DESC) AS `rank` FROM player_score) a WHERE a.rank = 1;
RENAME TABLE player_score TO player_score_old, player_score_new TO player_score;
DROP TABLE player_score_old;

https://github.com/karupanerura/isucon12-qualifier/blob/main/sql/pre_convert.sql

お気づきでしょうか。せっかくtalentごとにDB分けてるのにうっかりtenant_idを抜くのを忘れています。 ちょっともったいないですが些細なことなのでそのまま先に進みます。

このあたりを組み終わった頃に様子をみてみると、なかなかSQLiteからMySQLへの移行スクリプトが終わらないことに気付きます。 SHOW FULL PROCESSLIST を打って進捗を確認してみると、どうやらすごい量のplayer_scoreを持っているようです。

こりゃ大変だということで並列でインポートすることを試みます。 seq 1 100 | xargs -P10 -I{} bash -c './sql/sqlite3-to-sql ../initial_data/{}.db | mysql -uisucon isuports_tenant_{}' みたいな具合で並列で入れることにします。 この時点では、ろくに見積もりも取らずに、どうせ実装時間は取られるのだし並行で移行を進めておけば時間の無駄にはならなかろうと高をくくっていました。見積もりくらい取るべきでした。

実装デバッグ祭り

このあたりの実装をいったん終えたのが15:21頃だったようです。

github.com

新しいスキーマに合わせていざベンチ実行!コケたー! というわけで修正まつりです。

様々な問題、考慮漏れが次々と見つかります。visit_historyのRedis移行に対するPOST /initializeへの対応漏れや、同様に初期化におけるテナントDBのDROP DATABASE漏れ。 さらには、IDのURL上での16進数表記との相互変換漏れや、数年ぶりに使うSQL::Makerの使い方のミスなど、実に様々な問題を発見しますが一向にFAILは解消されず、刻一刻と時間が過ぎていきます。

最後まで残った問題が 大会内のランキング取得: ページングなし,上限100件 GET /api/player/competition/9fa52466/ranking 大会のランキングの1位のプレイヤーが違います (want: 9fa524cb, got: 9fa5252f) tenant:et-vrj-1659142684 role:player playerID:9fa524cb competitionID:9fa52466 rankAfter: というものです。 新しく作成されたテナントのもので、WebUIで確認しようにもログインセッションの発行の仕方が分からず(事前にちゃんと確認しておけばよかった……)、バグの原因が分からぬままモンキーデバッグを繰り返し、タイムアップを迎え、敗北が確定しました。

感想戦

なお、ベンチが公開されたので追試しつつデバッグを進めたところ、ベンチマーカーのレスポンスのログからすべてのプレイヤーが同一のスコアであった場合のランキングが正しくなかった事がわかりました。

修正内容としては以下です。

https://github.com/karupanerura/isucon12-qualifier/compare/main...study#diff-500630450a78877efc40cad8ce8ee6addd162a6bbe0ccf2bc9cd8cfaeae750ecR2

row_num をDROPしていたのを残すようにして、 INDEX ranking_idx (competition_id, score DESC, row_num) のインデックスを貼ることでクエリ一発でいい感じに取れるようにしています。

結果はこんな感じでした。

ほか、tenant_idをDROPしたり、 ORDER BY created_at 向けにインデックスを貼ったり細かいことをしているのでその影響も多少はあるかもしれませんが、デバッグ用にアプリケーションのAccessLogを有効にしているのも入っているので影響はほぼトントンでしょう。

もしこれが17時くらいにできていれば、複数台構成にしたりログを切ったりすることで3万点台は確実に狙えただろうと思うと悔しい限りです。

KPT

  • KEEP
  • PROBLEM
    • 計測のツメが甘い
      • 焦ったのもあるがボトルネックとなっているプロセスすらちゃんと特定せずに手を出したのはダメだった
      • 効果が思ったほど出なかった打ち手はその原因をちゃんと追求するべき
    • 計測と改善のサイクルがちゃんと回せていない
      • "改善"をデカくしすぎた
      • ついでに変えるのをやりすぎ
        • 先回りが良い方向に働いた部分もあったはずだけど確実性を欠いている
        • 結果的に博打っぽい打ち手になってしまった
        • 効果がそんなに見込めない打ち手に時間を使ってしまった(e.g. IDの16進数→10進数変換)
    • 一気にたくさん変えすぎた
      • 変更を一気に入れすぎてデバッグのサイクルを素早く回せなかった
    • 動作確認環境として他のサーバーを活かせなかった
      • MySQLへの移行とかは遊んでるサーバーでやっておいて、その間にSQLiteのままできる改善を進めておけばよかった
      • これができていれば、ULIDにシュッと変えてみて試しにベンチ流してみる。といったことを気軽に試せたはず
  • TRY
    • 焦らずちゃんと計測する
      • 問題の原因をちゃんと明らかにしてから次へ進む
    • ボトルネック以外には手を出さない
      • 特に、IDの16進数→10進数変換とかやる必要なかった。コードの変更箇所多すぎて時間的なコスパ悪い打ち手だった
      • 本当に手軽にできそうかちゃんと考えてから取り組む
    • 動作確認環境として他のサーバーを活かす
      • DBのオペレーションとか、ベンチ回しやすいところではやらないほうが他の作業をブロックしないのでよい

感想

解いていて面白かったです!ありがとうございました!次こそは……。

*1:GoでのWebアプリケーションの開発経験の大半はGoogle CloudでCloud Firestore(Datastore mode)を使うことが殆どでRDBの出番がなかった。

GCSへのアクセスをIdentity Aware Proxyで制限したいのでProxyを作った

storybookを共有したいなど、GCSに静的ファイルを配置しつつもそれを限定したメンバーだけに見せたいような用途ではアクセスを簡単かつ確実に制限するために、その制限にIdentity Aware Proxyを使いたくなることがあります。 しかし、Identity Aware ProxyはBackend Serviceに紐付けることはできますが、Backend Bucketに紐付けることはできません。Cloud Armerも同様です。 そこで、Backend ServiceとしてGCSのコンテンツを配信できるように、Proxy Serverを作りました。

github.com

ghcr.io/karupanerura/gcsproxy:v0.0.1 にビルド済のイメージを置いてあるので、 これをそのまま(予め自身のProjectのArtifact Registryにコピーを配置した上で)Cloud Runにdeployすることで、 GCSを参照するProxyとして動かすことができます。

GCS_PROXY_BUCKET 環境変数でGCSのバケット名を指定すればとりあえず動きます。

ほか、URLのパスプレフィクスをどうにかしたい場合、たとえば /static/ 以下をCloud Load BalancerからルーティングしてGCSのルートを参照させたい場合は GCS_PROXY_PATH_PREFIX/static/ と設定すればよいようになっていたり、 /index.html を参照させたかったら GCS_PROXY_INDEX_FILEindex.html と設定すればよくなっていたりなど、細かい機能が少しだけ実装されています。

あと(複数のRangeは扱えないけれど)Range Requestに対応していたり、gzip圧縮済のコンテンツをAccept-Encodingに応じてうまく扱えたり、HEADリクエストを解釈できたりなど、地味な機能がいくつか付いています。

よかったら使ってください。こんなの使わなくてもこれでいけるよ的な情報もお待ちしています。

PerlNavigatorがすごい

年々とelispのメンテが雑になってきて、ついにはemacsclientがemacs serverにうまく接続できなくなってしまい、とはいえ普通にスタンドアロンで立ち上げると動くのでログも取れずに原因究明が難しく、もはやこのままでは引退も近いかと思われたので、悪あがきでVSCodeに手を出してみることにした。

Perl Mongerの端くれとして、まずはPerlが書ける環境を整えようと、とりあえず最近ちょっと話題になっていたPerlNavigatorをVSCodeと共にインストールしてみた。

github.com

ところがこいつがすごい。

シンタックスハイライトをいいかんじにやってくれるのはもちろんのこと、emacsではperldoc -lmした結果に飛べるelispを仕込んでおいた(たぶんid:sugyanさんあたりのelispから拝借したきがする)のを使っていたが、PerlNavigatorはinclude pathの設定を入れたらいい感じにロードしてジャンプできる。 asdf でPerlを入れてもいい感じにつかってくれているようにみえる。

さらに、flycheckみたいな感じでエラーをわかりやすいところに出してくれし、auto-complete相当の雑な補完もよしなにやってくれる。

それも、大した設定もせずにこれだけのことがあっさりとできてしまったのがすごい。以下が入れた設定。

{
    "perlnavigator": {
        "includePaths": [
            "lib",
            "local/lib/perl5"
        ]
    }
}

これだけです。 lib.pm 置き場、 local/lib/perl5 はCartonで入れたやつを読むためのパスという感じ。 実際はarchごとにXSをビルドしたものが入るパスを lib pragmaのように突っ込めばより良い感じになるんだろうけどめんどくさくてまだやっていない。

設定したのはこれだけだけど、VSCodeに不慣れでもちょっとコードを書くにはそんなに困らない環境があっさり出来上がってしまったので驚いた。Amon2+Anikiで作ったWebアプリがいい感じに書ける。

実はむかしもVSCodeに移行しようとしたことがあったんだけど、そのときはセットアップしようとしたものの色々とどうすればいいのかわからず、だるくなってemacsに戻ってしまったが、これならVSCodeでもいいかなというところまであっさり来れたのでPerlNavigatorはすごい。

YAPC::Japan::Online 2022が終わり、その先の未来について

YAPC::Japan::Online 2022が終わりました。

JPA代表あるいはYAPC::Japan::Online 2022の主催としてのコメントはこちらに書きました:

blog.yapcjapan.org

ここでは、個人的に考えていることを書いてみます。

コミュニティ主催のカンファレンスの価値はなんだったのか

なぜ、ぼくたちはカンファレンスに参加(した|していた)んでしょうか?
なぜ、ぼくたちは、どこかの誰かがやってくれる大きなカンファレンスではなく、自分たちのカンファレンスを作っているのでしょうか?

人それぞれいろいろな答えがあると思いますが、はっきり「これだ」といえる理由がある方はおそらく多くは無いのではないかと思います。ぼくも、なんとなくカンファレンスに参加するのは好きだし、得られるものが多い気がしているだけで、あまりうまく言語化できないです。

自分の場合はどうだろうと、自分がカンファレンスに求めているものを言語化するとしたらなんだろうと考えると「自分の興味を広げる場」が大事なことの一つになるのかなと思いました。
そして、自分の場合はその主催者でもありますが、それに労力を割く理由はまた違って、PerlとPerlコミュニティの人々が好きで、それらのために少しでもなにか出来ることをしたいということになるのかなと思います。

自分の興味を広げるということ

自分の興味を広げる場、というのは実はなかなか作るのが難しいものだと思います。

なにか新しいことに興味を持つときのことを考えると、もちろん最初はそれに興味を持っていないわけですから、なにかきっかけが必要になります。 多くの場合、それは会話やSNS、テレビなどのマスメディア、あるいは仕事や生活のなかで起きることになります。

ただ、いずれも自分で選ぶものなので、いつの間にか傾向が偏っていきます。

フィルターバブルという言葉を聞くようになりましたが、ご存知でしょうか。 これはパーソナライズされたレコメンドなどを通じた情報に触れ続ける環境に居続けることによって、興味関心の薄い情報に触れにくくなる現象と言われていると思います。

フィルターバブルはこの最たるものの1つだと思いますが、そうでなくとも情報源を選択する必要がある以上は多かれ少なかれ偏りをもたらしているのではないでしょうか。 たとえば、普段よく見るテレビ番組はほとんど固定化されていませんか?新しい雑誌を最も最近読んだのはいつでしょうか?意識しなければ、たいていは同じものを選ぶのではないかと思います。 興味の幅を広げようと思っているのであれば、このような選択はしないほうがよいかもしれません。

自分の考えでは、興味の幅を広げるためには、興味関心から「少し」ズレた選択をすることが必要だと思います。 たとえば、身近な例だと、少し違う道を通って町並みを見てみる、書店に足を運んで少し違う本や雑誌を読んでみる、関連する少し違う分野の勉強をしてみる、といったようなことです。

カンファレンスの良さ

ぼくがカンファレンスの場に感じる良いところは、興味関心から少しズレたものに出会えるところでです。

カンファレンスに参加しようと思うと、一定のまとまった時間をカンファレンスに割くことになります。 目当ての発表がある場合はそれを聞きますが、そうでない場合はあまり興味がなかった発表を聞いてみたり、廊下などで近くにいた知り合いと話したりすることになるでしょう。 あまり興味がなかった発表が予想外に面白くてそのトピックに興味を持ったり、知り合いが聞いた別の発表について聞いたら

そして、懇親会で近くにいた人にとりあえず話しかけてみたりすると、全く違う発表に興味を持っていることがわかったりします。なかには当日に発表をしていた人などもいるでしょう。 その発表や分野の魅了を語ってもらったりするうちに、自分の興味がいつの間にか広がっているということも珍しくないのではないでしょうか。 さらに、気が合う人であればその後も他のカンファレンスなどで何度も合うことになって、いつの間にか同僚や友達になってることすらあります。

そういった、自分の興味を広げるチャンスがカンファレンスには多く広がっており、同じカンファレンスに参加し続けるだけでも少しづつそれを広げていくことが出来ると思っています。 そんな知的好奇心を刺激する楽しみがカンファレンスにはあります。また、いわゆる勉強会もその縮小としての楽しみがあるように思えます。

特に、コミュニティ主催のものはよりカジュアルなコミュニケーションに重きを置いていたり、トピックの幅も少し広く持っているものが多く、よりそんな楽しみが多いような気がしています。 (もちろん、企業主催のものでも通ずる部分は多いですし、また違った良さや面白さもあると思います。)

一方で、コロナ禍でオンラインカンファレンスが広まった後、オンラインカンファレンスでは以前ほどカンファレンスを楽しめないことが多くなったような気がします。

オンラインカンファレンスの難しさ

なぜ、コロナ禍でオンラインカンファレンスが広まった後、以前ほどの盛り上がりがなくなってしまったのでしょうか?

個人的な所感として、まず、懇親会への参加がいままでよりも難しくなったように感じました。

ZoomやGoogle Meetなどで大人数で雑談をしたり、オンライン飲み会をしたことがある方はおそらく共感していただけると思いますが、 意外と遅延が影響してなのか発話のタイミングが難しかったり、イヤホンやスピーカーだけでは同じ方向から複数の人の声が聞こえてくるからか聞き分けるのが難しかったり、カメラとマイクだけでは気分がなかなか共有できなかったりすると思います。

どれもちょっとしたことではあるのですが、その積み重ねでどうしてもある種のコミュニケーションの緊張感というか、そういったものが解きほぐしきれず会話が弾まない。会話が広がりにくいところがある気がします。

次に思い浮かんだものは、以前id:matsumoto_rさんが書いていたこの記事です。

hb.matsumoto-r.jp

自分は昨年に結婚をさせてもらったばかりで子供もいないのでまだ家庭内の仕事の総量は少ないほうだと思いますが、(パートナーがそこまで気にしていようといまいと)一定以上の負担をかけてしまう事実が自宅からオンラインで参加するとよりはっきりと分かるし、それがわかってしまえばそれを想像すると(オンラインでも、たとえオフラインに戻っても)二の足を踏みがちなのは気持ちとしてとても理解できます。(もちろん、それに何らか折り合いを付けて参加している方もおおぜいいらっしゃると思います。)

さらに懇親会といういわゆるサブイベントともなれば、ちょっとやめておこうという方も当然いらっしゃることでしょう。いい調子のときはただリラックスして楽しそうに喋ってるしそのように見えるものですから、パートナーからすると頭では分かっていても少しイラッとくる瞬間があるであろうことは想像できるでしょうし、そもそもの理解を得るにはコミュニティの文化的な背景とカンファレンス参加のモチベーションを共有する必要があるでしょう。

そして、一緒に参加している人の様子がわからないので、懇親会に参加してもどんな人たちと喋ることになるのかイメージが湧きにくいのも難しいポイントでしょう。

こういった様々なハードルがあってなのかはっきりとは分かりませんが、懇親会に参加する人はオンラインカンファレンスでは少ないようで、そうなるとやはり懇親会に参加する魅力はますます落ちてしまうでしょう。

このように、懇親会ひとつ取ってもこれだけ難しいので、オンラインカンファレンスには様々な難しさがあるといえます。

振り返ってYAPC::Japan::Online 2022ではどうだったのか

こういったことを感じていたので、目指したしたのが「ちゃんと交流できる」オンラインカンファレンスでした。

コミュニケーションの軸はDisocrdに置いて、いくつかの施策を実施したのですが、特に印象に残っているものを2つ紹介します。

裏トーク

まず、昨年にやったJapan.pm 2021でも同じことを目指して「裏トーク」というシステムを考えました。

これは、コミュニティの人を2~3人ほどMCとして招いて、カンファレンスの発表を聞きながらざっくばらんに語りあってもらい、それを参加者でトークの副音声として聞くことができるという仕組みです。 これによって、プラスアルファの楽しみを感じてもらいながら「空気感」を感じてもらうのが狙いでした。

どんな人がいるのかよくわからないところに飛び込むのは怖いものです。 こんな人達がいるのか、ということが人や空気感からなんとなく想像できると、少しだけかもしれませんが安心できるのではないでしょうか。

これがどれだけうまく行ったか、自分がフラットに体験することは叶いませんが、参加者の方々の反応を見る限りは上々だったのではないかなと想像しています。というか、そう思いたい。

前回で手応えを感じたので、今回も同様にやることにしました:

blog.yapcjapan.org

YAPCチキン && ビール && ラムネ

これは今回のためにスタッフとして手を上げてくれたメンバーが提案してくれたのですが、やはり飲食は強くて、同じご飯を食べるだけでもTwitterやDiscordなどを通じて空気感を共有できます。

ゲスト対談をしながら進行したのも相まって、懇親会は予想を大幅に上回る盛り上がりを見せて、自分は配信現場から翌日のために素早く撤収したりなどしてその環に混ざることが叶いませんでしたが、とても楽しんでもらえるオンラインカンファレンスの懇親会にできたのではないかなと思います。

紹介しきれないくらい様々なアイディアが出て、今回は少なくとも自分だけでは作ることができなかった、とても良いオンラインカンファレンスになったのではないかなと思います。

その先の未来について

来年のことを言えば鬼が笑うと言いますが、その先の未来についても少し想像を膨らませてみたいと思います。

YAPC::Kyoto 2020の延期判断をしてから実に2年もの月日が経ちました。 YAPC::Kyotoはまだ開催することができておらず、その見通しもまだ立っていません。

ただ、希望は出てきました。

ワクチンや治療薬の開発が進み実際に接種・投与がなされたり、さらに自宅療養も含めて医療的な知見が蓄積して、COVID-19に対して少しづつ医療的に立ち向かえるようになってきていること。コロナ禍における音楽フェスなどの大規模イベントの開催などを通じて、出来る限りの対策をした上で開催されるそれなりの規模のイベントに対してのリスクがその参加者のリテラシーと合わせて見直され、ものによっては社会的な反発が少なくなりつつあること。COVID-19の感染対策への個々人の向き合い方について一定の社会的常識ができつつあること。などです。

もともとの社会に戻ってきつつあるというわけではありませんが、オフラインでカンファレンスを開催する上でのハードルは少しづつ解消されていっているのではないでしょうか。

その一方で、一定の不可逆な変化もあるでしょう。

コロナ禍においてリモートワークが中心となり、生活スタイルが変化したこと。先に紹介した記事のように、それによって意識が変わったこと。様々な事情で引き続きCOVID-19への感染リスクが高い人がいるなかでウィルスの根絶はおそらく困難であることから感染対策が全く必要がなくなる可能性はおそらく低く、少なくとも中長期的にそれを継続する必要があること。などです。

オフラインでカンファレンスを開催することができたとしても、それに安心して誰もが参加することができるかというと必ずしもそうではなく、各々が何らかのハードルを乗り越えてくる必要があると想像しています。 いったいどれだけの人がそのハードルを乗り越えることができるのか、これだけ大きな変化があったわけですので、それを予想するのは困難になってきていると思います。

個人的な話

結婚して、将来について考えるなかで、人生の厳しさ・難しさを実感することが増えたように思います。 フィナンシャルプランナーに相談したり、家について考えたりなどすると、もっと頑張らなければと思わされます。

その一方で、カンファレンスの運営業などは報酬などの発生しない、ボランティアで行っています。

一般的に、コミュニティ主催のカンファレンスの運営は業務としては行わせてもらえないことが多いので、 業務外で家庭内などプライベートでもやらなければならないことが多いなかで、その隙間をうまく使ったり時間を捻出したりしてカンファレンスの準備に充てる必要があります。

しかし、カンファレンスを主催する立場ともなれば、多くの意識や注意をカンファレンスに注ぎ込まなければなりません。 十分にリソースを割かなければ、大したことができません。

たとえば、タスクを洗い出してスケジュールを決めたり、決まったことをスケジュールどおりにただやるだけではなく、状況に応じて柔軟に素早く意思決定をする必要があります。 そのためには、その背景に関する理解やチームに対する理解がなければならず、日々変化していくそれを自らインプットし、判断するべきタイミングを見逃さずにそれを発見し判断していくことが必要です。

少なくとも、自分がそういったことをちゃんと行うには、苦手なりにそれなりの時間をそこに費やして頭を整理して集中して考えなければ、まだまだそんなことはできそうにありません。

その一方で、カンファレンス運営にリソースを割きすぎると、当然ですが本業に影響をきたします。 うまくバランスが取れないと、どちらにも迷惑をかけてしまいます。

実際、今回は個人的にはそのバランスをうまく取れずどっちつかずな状態に陥ってしまい、それぞれで十分なバリューが発揮できなかったように感じます。 特に業務に目立って支障が出たとき、迷惑をかけてしまった同僚たちにはもちろんのこと、年に限られた回数しかない評価のことなど、考えると色々と苦い気持ちになります。

じゃあそんなに難しいのになんでやっているんだという話ですが、それはやはり僕がPerlとPerlコミュニティの人々が好きで、それらのために少しでもなにか出来ることをしたいというこの1点に尽きます。 そして、その気持ちだけで何も行動をしなければ、同じように他のひとも行動を起こさなくて、そのコミュニティやカンファレンスはあっさりと無くなったりあるいは風化してしまうという現実もよく知っているからです。

だからこそ、そういった難しさと向き合いながらも、YAPC::Japanというカンファレンスに関わり続けてきているわけです。

ただ、その気持ちと実際の負担に折り合いが付かないタイミングが来たら、どうしても関わり方を見直すことにならざるを得ない場面がいずれくるのだろうとも想像しています。

その先の未来

さて、暗い未来を想像しましたが、確かな希望もあって、それはこんな記事をここまで読んでくれている皆さんの存在です。

今回のYAPC::Japan::Online 2022では、自分にできることの限界をよく思い知らされました。 そして同時に、自分にない発想をもたらして、それを実現に向けて奔走してくれるスタッフのありがたさをです。

こういった活動を自分がこれからも続けていくためには、他でもない皆さんの助けが必要で、そしてそういった個々人の負担を減らしていくためにはまだまだ多くの方々の助けが必要であるということです。

もちろん、ぼくが苦しんだようなこういった難しさと多かれ少なかれ向き合う必要はあるかもしれないし、モチベーションの大小もひとそれぞれで、そしてみんな忙しくて、なかなか難しいことは分かっています。

それでも、どうか、少しだけでもいいので、力を、時間を、頭を貸してください。 もしよかったら yapc-japan-online-2022@googlegroups.com あるいは自分のTwitter DM宛で相談をください。

もちろん、カンファレンスは参加してくれる人、発表してくれる人なども居て初めて成立するものなので、無理に裏方を手伝ってもらう必要はないですし、これまで通り普通にカンファレンスを楽しんでもらうだけでもとてもありがたいです。

それでも、他のスタッフの参加ブログなどを読んで楽しそうだなとか少し興味を持つことができたなら、最初はお試し感覚でも大丈夫なのでご相談をいただけると嬉しいです。

PerlとPerlコミュニティの人々のためのそういった場や仕組みを、いっしょに色々なことを考えて実際に行動していって、未来をいっしょに作っていきましょう!

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

用語

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

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:Sixeightid: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/trendGET /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が終わったあとはさすがに疲労困憊したからなのか、それとも新しい処方に体が慣れてきたからなのか、ぐっすり眠れました。

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