Sider Blog

コードレビュー自動化サービス「Sider」を運営するSider株式会社のコーポレートブログです。

Sider 特別インタビュー GitHub アーロン・パターソン氏  コードレビューにとって大切なことは

2018年6月12日、13日の2日間にわたり、GitHub Satellite Tokyoが開催されました。 Siderでは、これにあわせて来日していたGitHub社のAaron Patterson(アーロン・パターソン)氏に、特別インタビューをお願いしました!気さくで日本語が堪能なアーロンさんは、終始笑顔で弊社CTOのSoutaroの色んな質問に日本語で答えてくれました。どうぞお楽しみください!

f:id:sideci:20180611122905j:plain  

GitHubでの日常 - RailsのアップグレードからGCの改善まで

Soutaro:
今GitHubでどんなことされてるのかをちょっと聞かせてもらえないかな、と思っています。

Aaron:
実はGitHubでは私は普通の開発者です。

Soutaro:
えっ??プロダクションのコードをバリバリ書いてるんですか!?

Aaron:
それは冗談。ジョーク(笑)。

GitHubでは私はメモリー使用量とウェブサイトの速度を改善しています。つまりパフォーマンスを中心に頑張っています。Rubyの開発だけをやるフルタイムコミッタではなくて、アプリケーションとRubyの両方を開発しています。

Soutaro:
RubyKaigiとかRubyConfとかで話していた、GCのやつとかですね。あれはGitHubで必要だったってことですか?

Aaron:
はい。GitHubのアプリケーションはすごくメモリを使うので、最近はRubyのメモリの使用量を減らす方法を考えています。

大体1年くらい前に、GitHub Enterpriseのサポートの人たちから「私たちのアプリケーションのメモリー使用量がだんだん増えてきた」と聞いて、それから調べ始めました。GHEとGitHub.comでは共有されているコードがありますが、GHEはお客様のハードウェアにインストールするものです。お客様のコンピューターのメモリはGitHub.comよりも少ないので、制約が厳しいんです。

Soutaro:
わかります。我々のSiderのオンプレ版も将来的にリソースが問題になると思っています。

Aaron:
私たちと同じ問題ですね。

Soutaro:
なんで最近あんなにメモリのことをずっとやってるのかなと思っていたんですよ。

RubyConf 2017で話されてたCompacting GCを僕はとてもすごいと思っていて、それまで「RubyでCompacting GCやってもほとんど意味ないんじゃ」って思っていたので、「あ、実際にできるんだ、効果あるんだ!」と思って。

Aaron:
ありがとうございます。開発は進んでいて、プロダクションにも入れてみたんですが、まだリリースしていません。バグがあるのでまだRubyコアにサブミットしてないんです。GitHubのプロダクションの一部で試したらバグが見つかったので、revertして。

GCの仕事はすごく楽しいけど、私の業務の割合としては20%くらいですね。普段はRailsの問題を修正しています。GitHubの開発で見つかったRailsの問題を直しています。

Soutaro:
同時にGitHubのコードも直していくっていう?

Aaron:
そうです。今のGitHubのRailsのバージョンは4.2ですが、1年前は3.2でした。今、5.2にアップグレードしている最中です。いろいろと問題が見つかってます。

Soutaro:
なるほど。入社されたときには3.2だったのを、これはアップグレードしないと駄目だって思って……

Aaron:
そうそう。プロダクションでRailsのmasterとRubyのtrunkを試したかったから、最初にアップグレードをしなくちゃならなかったんです。Railsのmasterを使えばアップグレードも簡単になると思います。いつもアップグレードしてくれるし、誰かがRailsにバグを入れたらすぐにわかるから(笑)。

Soutaro:
なるほど。それにしてもGitHubはすごいですね。RailsもRubyもわかってる人がチームにいて、直したRubyとRailsをすぐにプロダクションにデプロイできるっていうのは、すごく強いなと思います。

Aaron:
うん、強いと思います。

おそらくRubyだけを速くしても、Railsのことをわかっていないと結果はそこまで出ないと思うんです。今、WebアプリケーションのボトムラインはRubyというよりかはRailsなので。RubyとRailsの両方が理解できていると、もっと良い結果が出せるはずです。

Soutaro:
GitHubにジョインされることになったきっかけはどんな感じだったんですか?

Aaron:
GitHubは3回も入社を誘ってくれたんですよ。そのとき私はRubyのフルタイムコミッターになりたかったんですけど、GitHubは面接で「うちでそれができる」って言ってくれたんです。それが3回目の誘いだったので、もう4回目はないだろうと思って、入社しなくちゃならないと思いました。それが理由でした。

それから、入社を決めたもう一つの理由に、「GitHub.comのコードを見たかったから」というのもありました。

Soutaro:
GitHubのコードはどうでした?

Aaron:
結構問題あるけど……。今までにもっと悪いコードも沢山見たことあるから、実際はにはそんなに悪くないと思います。でも、もちろん問題ではあります(笑)。

面白かったのは、defunkt*1のバグを直したときですね(笑)。defunktが書いたバグを私が直したんです!つまらないバグでしたが、結構古いコードで、誰が書いたか知りたかったのでgit blameしたら、defunktが書いたのがわかった。

 
 

RubyとRails、スタイルに関して

Soutaro:
先日、Siderで松田明さんにインタビューをしたときにRubyとかRailsでのコードレビューとかコーディングスタイルについての話を聞いたんですが、何か補足とか、意見とか、これはウソでしょうみたいなことってありますか?

Aaron:
あの記事は面白かった。

コーディングスタイルはRailsとRubyは全然違いますね。例えばRailsではスペースだけを使っているけれど、Rubyの中ではスペースとタブを共に使ってます。

Soutaro:
あの記事を見て、Rubyコミッターの卜部(うらべ)さんが「Rubyも最近はスペースに統一することになってる」ってツイッターで言ってて。

Aaron:
本当!?

Soutaro:
えっ??

Aaron:
Wow! 知らなかった。

Soutaro:
この話をコミッター3人として、そのうち2人が知らないっていうのはどういうことだ……

Aaron:
スペースかタブかは私にはどっちでもいいので、その話自体を見てなかったです(笑)。Vimで自動でスタイルを合わせてくれるから、私には全然問題がない。

Vimは、設定すれば自動的にスペースかタブを選んでくれるんですよ。このVimの設定は特別な設定ですが、おそらくみんな同じファイルの中でスペースとタブをあんまり使ったりしないから、ほとんど知られていないんだと思います。 *2

Soutaro:
Railsはもう最初からスペースは2つなんですね。

Aaron:
スペース2つ。

でも好きじゃないところはprivateのキーワード。privateと書いた下にはさらに2つスペースを入れます。それは変だと思います。GitHubでは使わない。他のプロジェクトで見たこともあるけど、その開発者はもともとRailsの開発者でした。

Soutaro:
ちなみにDHHインデンテーションのメリットは何だと思いますか。

Aaron:
……分からない。

彼の主張では、「何がプライベートのメソッドか簡単に分かる」という点ですね。でもそのインデントがなくても、privateのキーワードを見たらすぐ分かると思います。その下はプライベートだから、これは意味がないと思う。

Soutaro:
ですよねえ。

Aaron:
スタイルの話で言えば、Seattle.rbのスタイルは括弧を使わない。

メソッドの宣言では引数に括弧を入れないんです。このスタイルはみんな嫌いなんですけど、私は好きです。なんでみんな嫌いなのかわからない。

Soutaro:
えっと、僕も嫌いです(笑)。

Aaron:
なんで嫌いなんですか?

Soutaro:
Cみたいな感じかな……うまく説明できないけど。

Aaron:
私が括弧を使わない理由は、RubyはLispみたいな言語ですが、括弧を使わないLispがかっこいいと思っているからです。Lispは括弧は多すぎるから。

Soutaro:
僕はOCamlも書くので、そのときの感じで書いちゃうのかも。OCamlは複数の引数をタプルで渡すか高階関数にするかの2通りがあって、型が違うんですよ。(int * int) -> intint -> int -> intの違いで。

Aaron:
ああ、なるほど。ここでは括弧の意味があるから。

Soutaro:
そうですね。あとは、Rubyだと「ここの括弧がなくても絶対に曖昧にはならないんだから書かなくていい」って聞いたことがあって、それはちょっと納得しました。

Aaron:
多分、みんなはC言語みたいななものに慣れているから、括弧がないと気持ち悪いんだと思います。

いずれにしても、一番重要なことはそのチームのスタイルを使うことです。Railsを開発するときはRailsのスタイル、GitHubを開発するときにはGitHubのスタイルを使っています。私の好きなスタイルはあるけど、それに対する私の個人的思い入れはそんなに強くないですね。

「もしも自分のスタイルを使いたいなら、自分のプロジェクトを開発したほうがいいですよ」と言いたい。

Soutaro:
プロジェクトによってスタイルが違うと、何か混乱したりとかしませんか?

Aaron:
あんまりない。だいたいのプロジェクトにはRuboCopとかがあるし、間違えたら誰かが教えてくれる。大丈夫です。

Soutaro:
RuboCopは好きですか?

Aaron:
実は、RuboCopは初めは嫌いだった(笑)。でも今は慣れました。

最初は「何で私にスタイルを押し付けてくるんだ」と思って嫌でした。でも考えてみれば、一番いいことは、同じチームのみんなが同じスタイルを使うことです。だから今はもう嫌いではないです。RuboCopを使うべき理由を分かっているから。RuboCopによってチームのスタイルが統一されることが大切。

Soutaro:
そうですよね。 Railsのレビューの話も聞かせてもらえますか?

Aaron:
ああ、いいよ。Railsのレビュープロセスは、私は簡単だと思う。レビューされていないから(笑)。

直接masterにプッシュしちゃうからレビューされない。もし間違えても、他の人が直してくれるから問題ないです。多分悪いことだけど…。

Soutaro:
Rubyみたいになってますね。

Aaron:
そう。最近PRを作るようになったけど、前は全然使わなかった。なんでかって言うと、PRを作るのが面倒くさいから。

でも、直接pushしてCIが落ちちゃったときにCIを直す方がすっごく面倒くさいことに気づいたので、今ではPRを作るようになりました。

 
 

GitHubの開発プロセス

Soutaro:
ここまでRailsとかRubyの話を聞いたんですけど、GitHub社内ではどんな感じでコードレビューをしてるんですか?

Aaron:
みんなとだいたい同じだと思います。普通はバグとフィーチャーを作るときにブランチを切って、変えて、開発をして、レビューをして、直して。

そのブランチは、プロダクションにデプロイして、問題がなければマージします。つまりmasterはいつもステイブルのブランチです。

Soutaro:
GitHubではPRをマージする前にデプロイする。

Aaron:
開発者がそれぞれフィーチャーブランチを切るんですけど、PRを出して、アプルーブされたらプロダクションにデプロイします。何も問題がなければそのままmasterにマージします。もしもエラーが増えるとかの問題があったら、masterをプロダクションにデプロイします。

デプロイのときは、まずは3%のマシンにデプロイされます。それでエラーの状況を見て、エラーが増えてなければ全部にデプロイ。その後でmasterにマージします。3%はトラフィックの絶対量としては多いけど、お客さんの割合としてはあんまり多くありません。なのでもし問題があったら簡単にロールバックできる。

つまり、masterは常に安定版になっていて、いつでもロールバックできるようにするための存在です。この辺は他の会社と違うと思います。

Soutaro:
はい。われわれは違いますね。PRをレビューして、マージしてからデプロイしています。ロールバックしたいときには、GitHubの画面からRevertしています。

Aaron:
なるほど。

Soutaro:
ちなみに、PRがアプルーブされてからデプロイ完了までってどのくらいの時間が掛かるんですか?

Aaron:
今、それは会社の中で大問題になっています。デプロイ自体は10分か20分で終わるんです。でも問題は、デプロイ待ちのキューで、キューで待たなくちゃならないということです。キューにはいつも10人とか5人くらいの人が入っていて、それぞれ10分か20分かかるから、1時間〜2時間待たないといけない。

でも私は今日本にいるから、時差のおかげでキューには誰もいない。みんな西海岸にいるから。今だったらデプロイは10分で終わります。

Soutaro:
すごい。デプロイし放題だ。

Aaron:
そう、そうです。デプロイの時間があまりかからないから。私はSlackで「みんな日本に引っ越したらいいよ」と言っています(笑)。

今、この問題を解決する方法をみんなで考えています。多分マイクロサービスとか何かを使うことになるのかと。アプリケーションが分かれれば、キューがたくさんになるから、待ち時間が短くなるはず。

Soutaro:
そうですね。今のはデプロイの話でしたが、レビュー自体は普通にやるって感じですか?

Aaron:
はい。RuboCopも使ってます。デフォルト設定は使ってないけど。公開されているGitHubのスタイルがあるので、そのスタイルを使っています。

でもGitHubのスタイルは、いつもダブルクォートを使うんです。私はいつもシングルクォートを使っていたから、入社したてのときにこれは面倒くさいと思った。RuboCopはいつも怒ってました。

Soutaro:
あれ?ダブルクォートはRailsも一緒じゃないですか?

Aaron:
Railsは、ダブルとかシングルのどっちでもいいと思います。

Soutaro:
そうだったっけ……?

Aaron:
そうかな?間違ってるかな……。

(この間Soutaroにhttps://github.com/rails/rails/blob/v5.2.0/.rubocop.yml#L120-L123 を見せられているAaron)

f:id:sideci:20180611121052j:plain

Aaron:
Oh, No!(笑)

Soutaro:
えー。何で気付かないんだろう……文字列リテラルをあんまり書かないんですかね。

Aaron:
あんまり書かないのかな……。

ダブルクォートと言えば、一般的なキーボードでダブルクォートを打つ時にシフトとクォートを押さなくちゃいけないのが面倒なので、私のキーボードで1つのキーを押したらダブルクォートが出せるようになってます。自分のキーボードで。

Soutaro:
えっ、すごい!

Aaron:
あと、私のキーボードだとホームポジションで括弧を書けるんです。このボタンを押して、DとFで括弧が出ます。これは括弧のオープニングとクロージング。

編集注:気になったので、後日アーロンさんのInstagramを覗きにいったら、自作キーボードの写真が色々載ってました。興味のある方はぜひどうぞ!

コードレビューで大切なこととは

Soutaro:
GitHubでの仕事のコードとRailsとかのオープンソースのコードでは、レビューに関してなにか違いはありますか?

Aaron:
私からすると違いはないですね。だけど、オープンソースのほうが良いレビューをしようとする気がします。ちゃんとコメントを書いて、フィードバックをあげる。企業での開発ではそうはいきません。みんな、コメントも短いし、レビューもあまりされなかったりする。

なので、私はオープンソースでレビューをするように、会社でも質の良い、丁寧なレビューをしようと心がけています。みんながそうしているかはわかりませんけど。私にとっての違いはその辺でしょうか。

Soutaro:
コメントの質が違ってくる理由は何だと思いますか?

Aaron:
会社だと同僚同士でお互いを知る機会があるので、もっと手っ取り早くレビューができてしまうんじゃないでしょうか。OSSでは、知り合いではなかったり違う企業で働くもの同士が協力しあうので、そうはいきません。もっとコミュニケーションを取る必要が出てくる。

でも私は、それは良いことだと思っています。なので、会社でも同じようにレビューしようと心がけています。新しく入ってくるメンバーに読んでもらいたい状況が出てくるかもしれないですし、後から見直したくなることだってあるかもしれない。自分でも忘れてしまうことがあるんだから、記録が残ってるほうが良いと思います。

あとは、企業とOSSの開発速度が違うのも関係するのではないでしょうか。会社での開発はOSSと違って時間があんまりないので、それがコメントのスタイルにも影響しているんだと思います。

Soutaro:
コードレビューをやっていく中で大事なことは何だと思いますか?

Aaron:
レビューで大事なことは、コードの変化を理解することです。そのコードがプログラムの挙動をどう変えているのか、何をしているのかが分かること、これが一番大切だと思います。でもdiffだけで見ていたら、そのことがわからない時もあります。そのあたりがPRでレビューをするうえでの難しい点だと思います。つまり、どうやって全容の理解に取り組むか、が大事です。

Soutaro:
僕はGitHubのPRはレビューの効率をすごく改善したと思うんですけど、まだ十分ではない?

Aaron:
レビューのコメントを書くことができたりとか、PRはすごく便利だと思います。

でもdiffだけ見ていると、どうやってプログラムが動くのかわからないときもある。diffを見たらコードの何が変わったかは分かりますけど、その変更が全体のシステムにどう影響するかがわからないことがあります。他のコードがどう影響するかがわからないから。それが自動でわかるようになるといいと思います。

Soutaro:
クラスの定義を変更したときに、他のコードから参照されているところで何かがおかしくなっていた、みたいな話?

Aaron:
そう。それが自動的で分かるとすごく便利。

Rubyで、なにかコードを解析してくれるものがあると、大きなプロジェクトですごく便利だと思う。

Soutaro:
型検査ツールとか?(笑)*3

Aaron:
そう。

でも……実は型を書きたくない(笑)。


f:id:sideci:20180611112743j:plain Aaronさん、お忙しいなかありがとうございました!

*1:編集者注: GitHubの共同創業者でありCEO

*2:編集注: 以下がその設定。https://github.com/tenderlove/dot_vim/blob/c5e3c7c1bb5273c9b4be4d1a6eb5f1312bd3915a/vimrc#L129-L133

*3:編集注:Soutaroが開発中の型検査ツールの話をしています