tech::hexagram

personal note for technical issue.

初めてOSSにコミットした話

mysql Elixir

mariaex というElixirのMySQLライブラリにPull Requestを送って、無事に3日前にmergeされた。初めてのOSS貢献で感慨深かったので記事に書いておこうと思う。

github.com

（※@hashijun は会社用のGithubアカウント）

きっかけ

今年の2月頃、業務中にとあるDBのテーブルを設計していて、業務ロジックから生まれる複雑なfieldをどう保存するか悩んでいた。関わっていたのはリリース前のとあるElixir製プロダクト。そのタイミングで保存すべきfieldを絞り込み、テーブルを設計してPull Requestを出した。schemaの中にカラム数は結構あった。

設計したテーブルをレビューしてもらったところ、「それ、まとめてJSONカラム使ったら良いんじゃない？」とベテランの同僚エンジニアから指摘を受けた。確かに、今後カジュアルにカラムを追加したり削除する上で都合が良いので、指摘に沿ってJSONカラムを利用することに。

そのプロダクトではMySQLのライブラリに mariaex を利用していたが、実装当初はJSONカラムがunsupportedだった。

github.com

関連するissueが立っており、repositoryのmaintainerが実装した commit が残っていた(Pull Requestは出ておらず)ため、これを元にmasterからブランチを切って差分を作ってみることに。

試行錯誤を重ねた上でPull Requestを出したが、mergeされるまですんなりとはいかなかった。

github.com

放置されることで、熟成されるconflict

上述のmaintainerのcommitを実装当初のmasterの上にcherry-pickし、メソッドのarityや呼び出され方を修正して1つのcommitにまとめた状態にしていた。そして手元でCIが通るかどうかの検証を行っておらず(手元の環境で mix test は走らせていたが TravisCI は動かしていなかった)、テストが落ちた状態でPull Requestを出してしまっていた。

Pull Requestを出してから諸々修正していたため、commitが乱雑な状態になってしまった。その結果maintainerやcontributorもレビューしづらい状態になっていたのか、誰からもコメントが付かずに数ヶ月が経過してしまった。

数ヶ月の間に、

  • .travis.yml が更新されテスト環境のMySQL/MariaDBのバージョンが複数定義されるようになった
  • Elixir 1.3で導入されたCalendar Typeのサポートが入り、自分が修正していたメソッドとほぼ同じ箇所に修正がガッツリ入った

ことによって、conflictが熟成されていった。

その結果、こんな感じの悪循環に陥りこのPull Requestはそっ閉じせざるをえず、あえなくお蔵入りとなってしまった。

f:id:manji602:20171112013352p:plain

この間ですでに6ヶ月弱が経過していた。

仕切り直しは慎重に

初めに作成したPull Requestでは、commitが意味のあるまとまりになっていなかったり、maintainerとコミュニケーションが密に取れていないことで放置されてしまっていたので、仕切り直して実装する際は下記の点に留意した。

  • commitを見やすい単位で分割する
  • Pull Requestを作成する前に、手元の環境でCIを通す
  • Pull Requestを作成した後も、適当なブランチを切ってCIが通ることを確認してからpushする
  • Pull Requestを作成して1週間コメントが付かなかったら、maintainerに見てもらう用メンションを飛ばす
    • 飛ばす予定だったが、実際はすぐコメントが付いたのでこれは不要に

仕切り直した結果、maintainerや他の開発者からもコメントが付き、ある意味「ホットな」Pull Requestになった。頂いた指摘にも早急に対処した結果、無事にsubmitから1ヶ月弱でmergeしてもらえた。

学んだこと

今回のOSS contributeでいくつか学びがあったので最後にまとめておく。

MariaDBのJSONはMySQLのJSONとは別モノ

mariaex では MySQL だけではなく MariaDB についてもサポートしている。 MariaDB では10.2以降にJSONカラムがサポートされた。

この記事をよく読まずに JSON カラムを利用したテーブルのテストをMariaDB 10.2以降の環境で出来るように実装していたが、下記のようにテストが落ちてしまっていた。

1) test select json (TextQueryTest)
   test/text_query_test.exs:106
   Assertion with == failed
   code: rows == [[%{"hoge" => "1", "huga" => "2"}], [%{"hoge" => "3", "huga" => "4"}]]
   lhs:  [["{\"hoge\": \"1\", \"huga\": \"2\"}"], ["{\"hoge\": \"3\", \"huga\": \"4\"}"]]
   rhs:  [[%{"hoge" => "1", "huga" => "2"}], [%{"hoge" => "3", "huga" => "4"}]]
   stacktrace:
     test/text_query_test.exs:110: (test)

2) test encode and decode json (QueryTest)
   test/query_test.exs:690
   Assertion with == failed
   code: query("SELECT map FROM #{table} WHERE id = 1", []) == [[map]]
   lhs:  [["{\"hoge\": \"1\", \"huga\": \"2\"}"]]
   rhs:  [[%{"hoge" => "1", "huga" => "2"}]]
   stacktrace:
     test/query_test.exs:702: (test)

色々と調べていると、MariaDBのJIRA にJSONサポート時の経緯がコメントされているのを見つけた。

Sergei Golubchik added a comment - 2017-05-29 19:11
...
Also, speed-wise MariaDB does not need binary JSON, according to our benchmarks, our JSON parser is as fast on text JSON as MySQL on binary JSON.

---

Sergei Golubchik added a comment - 2017-07-05 15:18
added JSON as an alias for TEXT

というわけで、 MariaDB に関してはJSONカラムはバイナリレベルではTEXTとして扱われてしまうので注意が必要ということを副次的に知ることとなった。

Pull Requestもコミュニケーション

いくら差分全体が良くても、何度もテストを直したり、masterをmergeしたcommitが乱雑に並んでいる状態ではレビューする側も見づらいし、それにより前述のような悪しき無限ループに陥ってしまう。

以前偉そうにQiitaに書いていたが、自分がそれを実践できていなかったので、猛省。

mergeされるまでに時間がかかるとconflictが発生する確率が上がっていくため、mergeしてほしいという強い意志を、maintainerにアピールしていったほうが吉ではないかと思う。

おまけ

mergeされた後についた コメント で気付かされたが、Pull Requestを作り直した際にクエリに含まれるJSONをエンコードできるようにする処理を入れそびれてしまったので、まだ続編がありそう…

次は会社用じゃなくて個人用のアカウントで実装しようかな。