PrestoのPull Requestのやりとりが興味深い

Prestoはチーム外からのPull Requestも積極的に取り入れておりオープンで大変に好感が持てる訳ですが、なかでもコードレビューがとても丁寧です。

例えば僕が最近だしたPull Requestだとこんな感じです。

Add url_encode/url_decode function by wyukawa · Pull Request #3235 · prestodb/presto · GitHub

こんだけちゃんとコメントしてくれるOSSなんて他にあるのかなと思うぐらいすごいです。

ちなみにPrestoチームはGitHubのmergeボタンは使ってなさそうです。
コミット履歴を見ても外から見えるのはgh-pagesを除けばmasterブランチ１本の一直線の履歴です。

もらったPull Requestを1つにまとめて取り込んでいるようです。
コミットのAuthorがPull Requestした人でCommitterがPrestoチームの人です。
例えば僕のPull Requestは2回のコミットがありますが、下記のように1つにまとめられています。

他に面白かったPull Requestがこちら
Fix InvalidRange error and SocketTimeoutException in PrestoS3FileSystem by y-lan · Pull Request #2647 · prestodb/presto · GitHub

2番目のコメントがこちら

Is there any reason to log this? We normally avoid logging unless it will be useful to the administrator, as these sorts of things can happen many times per second and thus can easily flood the log with useless information.

ふうむ。何をロギングしないかも大事ですね。

まあ確かにPrestoのような大量データを扱う分散システムだとログが溢れて大事なログを見失ったりHDD容量を逼迫させたりする危険性があるので、なんでもかんでもロギングすれば良いってもんじゃないですね。

他にも

Thanks for sending this PR. Overall this looks good. Since these two changes are logically separate, can you split this up into two separate commits?

とかあってコード以外にコミット粒度の話まで出てきます。
このケースは特殊だと思うんですけど1回のPull Requesが最終的には2つのコミットになって取り込まれています。

どちらかというと反面教師として見た方が良さそうなPull Requestがこちら
Adding Elasticsearch connector. Please check and suggest improvements. by sumanth232 · Pull Request #3240 · prestodb/presto · GitHub

コメントを見ればわかるんですが、System.out.printlnとかjava.util.*とかe.printStackTrace()とかにたいして細かく指摘してます。

そういう指摘はされないようにしてからPull Requestするのがマナーだと思いますが、丁寧に相手してますね。

あとPull Requestに限らずIssueでも自分のユースケースを説明するのが大事ですね。

僕が出したUDF追加のPull Requestなんかはユースケースが割と明らかなので説明する必要もそんなに無いですけど、そうでない場合はなんでその機能が必要なのかを伝えるのが大事だと思います。

例えば、僕が以前コメントしたAdd support for implicit cross joins · Issue #858 · prestodb/presto · GitHubなんかはその辺が伝わったから実装されたかなと勝手に思ってます。