今川館

都内勤務の地味OLです

俺的embulkでMySQLにデータを入れるときの注意点

最近、embulkをいじる機会があって、色々と思うところがあったので書いておく。
今回わたしが使ったケースではembulkのINPUTはバッチが作ったJSONL形式のファイルで、OUTPUTがMySQLのDBである。
OUTPUT先のDBのデータはAPIから専ら参照用に作られ、APIがそのデータを更新することはない。

以上の背景から思ったことを以下書いていく。

INT AUTO_INCREMENTのサロゲートキーをやめた

一般に、テーブルにはINTやBIGINTのAUTO_INCREMENTを指定されたサロゲートキーを設けるのが定石とされる。
DjangoとかRuby on Railsなどを使っていると、サロゲートキーはごくありふれたものだろう。

だが、embulkで投入するテーブルにはサロゲートキーを設けない方が良いと思った。
特に、INT型で作るのはヤバイと思う。

なぜかというと、embulkは定期的に件数の多いデータを入れる道具なので、頻繁にデータ更新が行われ、AUTO_INCREMENTも頻繁に行われるのであっという間にINTの最大値を超えてしまうからである。

INTの最大値なんて2,147,483,647(21.4億)なので、100万レコードの挿入を5分おきに実行していたら7.4日程度でオーバーフローしてしまう(!)*1

UNIQUE制約の作成はafter_loadで行うようにした

embulk-output-mysqlプラグインではafter_loadという、OUTPUTの処理が行われた後に実行すべきSQL文を書いておくことができる。

参考: https://github.com/embulk/embulk-output-jdbc/tree/master/embulk-output-mysql#configuration

after_load: if set, this SQL will be executed after loading all records.

after_load: これが設定されると、すべてのデータを投入後に設定されたSQLが実行されます。

embulkで大量件数のデータを投入することを想定すると、CREATE TABLEのタイミングではUNIQUE制約を作らず、全データ投入後に作るようにした。
最初からUNIQUEインデックスがあると挿入のコストが嵩むので、全部データを入れた後に一回にまとめてUNIQUEインデックスを作った方がトータルのコストは安くつくからである。

なお、当初はBOOL型の列に(0 OR 1)のCHECK制約を作っていたのだが、そもそもCHECK制約自体をやめてしまった。
元々のINPUTの方でこれら値がクリーンであることが保証されている(または期待される)ならば、OUTPUTの方でいちいちチェックするコストを削ってしまおうと思ったからである。

でも主キーは作っておかないとダメ

UNIQUEインデックスやサロゲートキーを作らないのは良かったのだが、同じ発想で最初は主キーを作るのもやめていた。
ところがMySQLは主キーが指定されていないテーブルに対しては裏で独自の擬似主キー的なものを作って管理するので、結果的に主キーを指定しないことにコスト的な利点はないらしい。

参考: https://dev.mysql.com/doc/refman/5.7/en/innodb-index-types.html

If the table has no PRIMARY KEY or suitable UNIQUE index, InnoDB internally generates a hidden clustered index named GEN_CLUST_INDEX on a synthetic column containing row ID values.

そのテーブルに主キーもユニークインデックスも指定されていないと、InnoDBは内部で隠しクラスターインデックスを作ります。
それは既定の列の値を合成した値を使用し、GEN_CLUST_INDEXという名前がつけられます。

modeはreplaceとtruncate_insertのどちらが良いのか?

OUTPUTのmodeはいくつか種類があって、採用候補にしたのが以下の二つだった。

  • replaceモード: CREATE TABLE/ DROP TABLEを行い、テーブルを丸ごと入れ替える方法
  • truncate_insertモード: 予め一時テーブルに全データを挿入しておき、後で本物のテーブルをDELETE/ SELECT-INSERTする方法

どっちが良いのかは少しもやもやと悩んだ挙句、今回はreplaceモードを採用した。
理由は特にここで述べるほど強い根拠があるわけではなく、なんとなくという程度である。

それより、TRUNCATE文を実行しないなら「truncate_insert」という名前はやめたら? と思ってしまった(小言

TRUNCATEしないならインデックスの一括クリアもAUTO_INCREMENT値のリセットも行われないし、ロールバックは可能なわけで、違う印象を持ってしまうので良くないと思う。


最後までお読みくださりありがとうございました。

*1:これ開発中にふとしたきっかけで気づいて本当に助かったと思う。マジでヤバイと思う。

Goはnilをレシーバーにメソッドを呼んでもnil pointerで落ちない

Goはヌルポしないという事実に驚愕してしまった

もう言いたいことはタイトルがすべてなのだが、Goはnilからメソッドを呼んでもnil ponterでパニックになったりしない。

以下のGoのプログラムでmain()関数のローカル変数fはnilだが、f.IsNull()などのメソッドを呼んでもパニックにならず結果を返す。

package main

import (
	"fmt"
)

type Foo struct {
	Name string
}

func (f *Foo) IsNull() bool {
	return f == nil
}

func (f *Foo) Hello() string {
	return "Hello, world."
}

func main() {
	var f *Foo
	fmt.Println(f.IsNull(), f.Hello(), f) // => true Hello, world. <nil>
}

これには驚愕してしまった!

もしかしたら最近の言語は新しいバージョンに変わってヌルポ対策されてヌルチェックが必要なくなったのかと思った。

Pythonで同じことをやってみた

気になってPythonでも試してみた。

class Foo:
    def is_null(self):
        return self is None


f: Foo = Foo()
print(f.is_null())  # False


x: Foo = None
print(x.is_null())

typeヒントをつけて似たようなことをしたが、当然のようにAttributeErrorが出る。違和感なし。

実行結果

Traceback (most recent call last):
  File "foo.py", line 12, in <module>
    print(x.is_null())
AttributeError: 'NoneType' object has no attribute 'is_null'

ちなみにPythonのバージョンは3.7.4だった。

$ python3 -V
Python 3.7.4

まあPythonは型がない言語だから(審議中)こういうところはいい加減なのかもしれない。

Javaでも同じことをやってみた

でもJavaはどうなったのか気になる。
何と言ってもヌルポはJavaが生んだと言っても過言ではないほどヌルチェックばかりやらされた昔の嫌な思い出しかない。
もし新しいJavaでヌルポ対策されていたなら世の中は良くなっていないと誰が言えるであろうか(反語)。

結論から言うと、JavaはおなじみのNullPointerExceptionが出てしまった。

public class Foo {
    public static void main(String[] args) {
        System.out.println("Hello, world.");    

        Person p = new Person();
        System.out.println(p.isNull()); // => false

        Person p2 = null;
        System.out.println(p2.isNull());
        // Exception in thread "main" java.lang.NullPointerException
        // 	at Foo.main(Foo.java:9)
    } 
}

class Person {
    boolean isNull() {
        return this == null;
    }
}

なーんだ〜〜〜、やっぱりいつものJavaじゃないか〜〜〜〜wwwwww

ちゃんちゃん、世の中甘くないのである。

ちなみにJavaのバージョンはopenjdk12だった。

$ java --version

openjdk 12.0.2 2019-07-16
OpenJDK Runtime Environment (build 12.0.2+10)
OpenJDK 64-Bit Server VM (build 12.0.2+10, mixed mode, sharing)

サンプルコードは↓ここに置いておきました。

https://github.com/oyakata/gooooolang/tree/master/javanullpo


最後までお読みくださりありがとうございました。

コードフォーマッター「black」を使ってPythonのコードの整形で悩むのをやめる

Pythonにもblackというgofmtみたいなコードフォーマッターがあった


Goを書いていると、gofmtコマンドを叩くとインデントやTrailing whitespaceなどを勝手に直してファイルを書き換えてくれる。
わたしがPythonを使ってきてGoを覚え始めたときにこれは便利だなぁと思った。

知らなかったことだが、最近はPythonにもblackというコードフォーマッターがあるらしい。

black · PyPI

これを使うと以下のようなPythonのプログラムを勝手に整形してくれる。

[before]

from pathlib import Path


def main():     
    path = Path(__file__).parent.joinpath("foo", 'bar', "baz_1234567890123456789012345678901234567890123456789012345678901234567890")
    print(path) # ここにプリントされる

if __name__ == '__main__':
    main()

[after]

from pathlib import Path


def main():
    path = Path(__file__).parent.joinpath(
        "foo",
        "bar",
        "baz_1234567890123456789012345678901234567890123456789012345678901234567890",
    )
    print(path)  # ここにプリントされる


if __name__ == "__main__":
    main()

具体的に何を整形してくれたかというと、

  • トップレベルの空行は1行ではなく2行のルールなので、1行しか空けていないところを2行に増やした
  • ラインコメントの"#"の前は半角スペースを二つ空けるルールなので、ひとつだったのを二つに増やした
  • main()関数のコロンの後ろに存在した半角スペース(=Trailing whitespace)を消した
  • シングルクォートで囲んだ文字列をダブルクォートで囲むよう変えた
  • 1行が88文字に(なぜ79文字でないのかは後述)収まらない箇所を適宜改行した
  • 改行区切りで関数の引数を記述する際、ケツカンマをつけた

だいたいこんな感じ。

もうpep8だのpyflakesだのflake8なんてものをいちいち叩いて自分でコードを整形する必要がなくなったのだ。

blackは1行の文字数を88文字にする慣わしらしい

pep8だと1行の文字数は79文字以内と決まっているので、flake8もこれに合わせて79文字以内かどうかチェックする。
ただし、文字数は設定で変更できるようになっている。

blackは88文字がちょうどいいとblackを作った人たちが思っているらしく、88文字以内に収まるようコードを整形する。

それが嫌な場合は.flake8に設定を書いて対処するようblackのREADMEに書いてある。

https://github.com/psf/black#line-length

blackをインストールするときのハマりポイント

poetryで入れるときは allow-prerelease オプションをつけないと(今のところ)ダメだった

poetryを使ってblackを入れようと以下のコマンドを叩いたらエラーになった。

$ poetry add black
                                                      
[ValueError]                           
Could not find a matching version of package black  
                                                      
add [-D|--dev] [--git GIT] [--path PATH] [-E|--extras EXTRAS] [--optional] [--python PYTHON] [--platform PLATFORM] [--allow-prereleases] [--dry-run] [--] <name> (<name>)...

これは最初意味がわからずググったら以下のスレッドが見つかって、 --allow-prerelease というオプションをつけないとダメらしい。

Can not install black · Issue #649 · sdispater/poetry · GitHub

オプションをつけたら今度はうまくいった。

$ poetry add black --allow-prereleases
Using version ^18.3-alpha.0 for black

Updating dependencies
Resolving dependencies... (1.9s)

Writing lock file


Package operations: 0 installs, 1 update, 0 removals

  - Updating black (19.3b0 -> 18.9b0)
root@1d215d0967e6:/code# poetry lock
Updating dependencies
Resolving dependencies... (0.1s)

ちなみにpyproject.tomlを見ると以下のように「allows-prereleases = true」という記載が付いていることがわかる。

black = {version = "^18.3-alpha.0", allows-prereleases = true}

正式リリースされたらこんなオプションをつけなくても良くなるのだろう。

サンプルコード

サンプルコードはここに置いておきました。

https://github.com/oyakata/pythooooon/tree/master/blaccccck

なお、GoよりもPythonの方が実行環境作るの面倒臭いので、一応実行方法をここに書いておきます。
(dockerが入っている環境で動かしてください)

$ git clone git@github.com:oyakata/pythooooon.git
$ cd pythooooon/blaccccck
$ make black

Dockerfileとかdocker-compose.ymlでコンテナ立ち上げたり、コンテナの中でpoetry叩くコマンドを覚えるのも面倒なので、Makefileに手順を書いておきました。


最後までお読みくださりありとうございました。

mapの競合状態のはなし

目次

mapの競合状態とは何か?

GoのmapはあるgoroutineでReadしているときに別のgoroutineからWriteしてはいけないというルールがある。

もし違反するとpanicを起こす。

このことはgo1.6のリリースノートに書いてある(他の出典を探したけど見つけられなかった・・)

Go 1.6 Release Notes - The Go Programming Language

if one goroutine is writing to a map, no other goroutine should be reading or writing the map concurrently. If the runtime detects this condition, it prints a diagnosis and crashes the program.

# あるgoroutineがmapに書き込んでいるとき、別のgoroutineが同じmapを読んだり書いたりするとランタイムはこの競合状態を検出します。
# そして、その旨を表示してプログラムをクラッシュさせます。

だから、こういうプログラムを書いて動かすとパニックで異常終了する。

package main

import (
	"fmt"
	"math/rand"
)

func main() {
	m := map[int]struct{}{}
	go func() {
		for {
			m[rand.Intn(100000)] = struct{}{}
		}
	}()

	for {
		fmt.Println(m[rand.Intn(100000)])
	}
}

実行結果

fatal error: concurrent map read and map write

単にキーを渡して読み取るだけでなく、rangeでmapをループしてもダメである。

package main

import (
	"fmt"
	"math/rand"
)

func main() {
	m := map[int]struct{}{}
	go func() {
		for {
			m[rand.Intn(100000)] = struct{}{}
		}
	}()

	for {
		for k, v := range m {
			fmt.Println(k, v)
		}
	}
}

実行結果

fatal error: concurrent map iteration and map write

どうしたらいいのか?

sync.RWMutexを使う

syncパッケージのRWMutexを使ってmapへアクセスする前にロックを取ればこの問題を回避できる。

先ほどのプログラムでmapに要素を入れたり、要素を取り出す前後にロックの取得と解放を行うよう改めると、パニックを起こさなくなる。

[このプログラムは途中で異常終了しないので、やめるときはCtrl+Cで落とす]

package main

import (
	"fmt"
	"math/rand"
	"sync"
)

func main() {
	m := map[int]struct{}{}
	var mu sync.RWMutex

	go func() {
		for {
			mu.Lock()
			m[rand.Intn(100000)] = struct{}{}
			mu.Unlock()
		}
	}()

	for {
		mu.RLock()
		v, ok := m[rand.Intn(100000)]
		fmt.Println(v, ok) // {} true と {} false が混ざって表示されるが、mから要素を消さないので次第に {} true ばっかり表示されるようになる
		mu.RUnlock()
	}
}
共有ロックと排他ロック

RWMutexはLock/Unlock(排他ロック)とRLock/RUnlock(共有ロック)のロック用メソッドがある。

共有ロックとは - IT用語辞典 e-Words

このため、Lockの呼び出し時にLockないしRLockが既に呼ばれていた場合、そのgoroutineはロックの解放を待つ。
しかし、RLockの呼び出し時はLockが呼ばれていない限り待たずに処理を先へ進める。

sync.Mapを使う

syncパッケージのMapを使うとこの競合状態に対策してRead/Writeしてくれる。
ロックの取得と解放処理はプログラムの状態に気を使って書かなければならない(=難しい)ので、mapを置き換えたいだけならこっちの方がお手軽である。

元のプログラムでmapを使っていた部分をsync.Mapに取り替えると今度はパニックを起こさない。

[このプログラムも、やめるときはCtrl+Cで落とす]

package main

import (
	"fmt"
	"math/rand"
	"sync"
)

func main() {
	var m sync.Map

	go func() {
		for {
			m.Store(rand.Intn(100000), struct{}{})
		}
	}()

	for {
		fmt.Println(m.Load(rand.Intn(100000)))
	}
}

atomic.Valueを使う

sync/atomicを使う方法もある。

例えば、mapに格納するデータは定期的にMySQLのテーブルを全件取得して丸ごと入れ替えるといった方式をとる場合、sync.Mapではなくatomic.Valueにmapを入れても安全にデータを読み書きできる(以下)。


[このプログラムも、やめるときはCtrl+Cで落とす]

package main

import (
	"fmt"
	"math/rand"
	"sync/atomic"
)

func selectFromMySQL() map[int]struct{} {
	// データの全取っ替え。
	// ここではランダムにやっているけど、実際はバッチが定期的に更新するMySQLのテーブルをSELECTすると思ってください。
	size := rand.Intn(100)
	m := make(map[int]struct{}, size)
	for i := 0; i < size; i++ {
		m[i] = struct{}{}
	}
	return m
}

func main() {
	var at atomic.Value
	at.Store(selectFromMySQL())

	go func() {
		for {
			at.Store(selectFromMySQL())
		}
	}()

	for {
		m := at.Load().(map[int]struct{}) // Loadはinterface{}型を返すので型アサーションが必要
		v, ok := m[50]
		fmt.Println(v, ok) // {} true と {} false が混ざって表示される
	}
}
ロックとCompare and swap

なお、RWMutexやsync.Mapはロックで状態を守っているが、atomic.ValueはCompare and swap(CAS)という違う方法で守っているらしい。

コンペア・アンド・スワップ - Wikipedia

さいごに 感想

ここまで説明されると「ふーん」という感じで特に感慨もないが、実際はmapの競合状態に起因するバグを仕込んでしまうと発覚や修正が難しい。

例えばmapをインメモリキャッシュとして使うサーバーを作ってこのような問題を起こすバグがあっても

  • mapの競合状態を検出してサーバーがクラッシュする
  • 監視ツールがサーバープロセスのダウンを検知して再起動させる
  • サーバー再起動する
  • またmapの競合状態が発生し、クラッシュする
  • 監視ツールが再起動させる… この繰り返し

こういう風に監視ツールが再起動させるよう設定が入っていたりすると、単純にサーバーがダウンするんじゃなくて、クラッシュ通知も来ないし裏では再起動を繰り返してサーバーの応答性能が劇落ちしてるといった判断の難しい状況に陥ることもある(実際あった)。

サーバーを複数台立てて冗長化しているとなおさら問題に気付きにくくなる。

だから競合状態(Race condition)はこわい。

サンプルコードはここに置いておきました。ちょっと雑だけど・・

https://github.com/oyakata/gooooolang/tree/master/maprace


最後までお読みくださりありとうございました。

NULLを許可する列をScanするときの注意点

NULLを許可する列をScanするときは要注意

database/sqlMySQLのデータを取得するとき、NULLを許可する列の扱いがちょっと要注意だった。

まず、こういうNULLを許すテーブルにデータを入れる。

CREATE TABLE `students` (
  id INT PRIMARY KEY,
  active TINYINT NULL,
  name VARCHAR(255) NULL,
  grade INT NULL,
  score DOUBLE NULL
) ENGINE=InnoDB DEFAULT CHARSET=utf8;

INSERT INTO `students` (id, active, name, grade, score) VALUES
  (1, 1, 'John Doe', 65535, 0.009876),
  (2, NULL, NULL, NULL, NULL),
  (3, NULL, 'Akira Toriyama', NULL, 100)
;

これを検索して、*Rows.Scan()を呼ぶとNULLを読み出してエラーを返す。

type Student struct {
	ID     int
	Active bool
	Name   string
	Grade  int
	Score  float64
}

rows, err := db.Query(`SELECT id, active, name, grade, score FROM students`)
if err != nil {
	log.Fatal(err)
}
defer rows.Close()

for rows.Next() {
	s := Student{}
	if err := rows.Scan(&s.ID, &s.Active, &s.Name, &s.Grade, &s.Score); err != nil {
		log.Fatal(err) // ここでエラー
	}
	log.Println(s)
}

コンソール出力を見ると、1行目は問題なくプリントできるが、2行目のデータがactiveにNULLが入っているため、下記の通りプログラムが異常終了する。

2019/08/12 08:45:26 {1 true John Doe 65535 0.009876}
2019/08/12 08:45:26 sql: Scan error on column index 1, name "active": sql/driver: couldn't convert <nil> (<nil>) into type bool

database/sqlのNullBool, NullString, NullInt64, NullFloat64を使う

database/sqlパッケージを見ると、このようにNULL列をScanするときに使うべきものが定義されている。

https://golang.org/pkg/database/sql/#NullBool

さきほどのプログラムをNullBool, NullString, NullInt64, NullFloat64を使うよう変えると今度はエラーにならない。

type NullableStudent struct {
	ID     int
	Active sql.NullBool
	Name   sql.NullString
	Grade  sql.NullInt64
	Score  sql.NullFloat64
}

rows, err := db.Query(`SELECT id, active, name, grade, score FROM students`)
if err != nil {
	log.Fatal(err)
}
defer rows.Close()

for rows.Next() {
	s := NullableStudent{}
	if err := rows.Scan(&s.ID, &s.Active, &s.Name, &s.Grade, &s.Score); err != nil {
		log.Fatal(err)
	}
	log.Println(s)
}

出力結果

2019/08/12 09:03:49 {1 {true true} {John Doe true} {65535 true} {0.009876 true}}
2019/08/12 09:03:49 {2 {false false} { false} {0 false} {0 false}}
2019/08/12 09:03:49 {3 {false false} {Akira Toriyama true} {0 false} {100 true}}

NullXXから値を取り出す方法

ただ、最初のプログラムと後のものの出力内容を比べるとちょっと違いがある。

最初の出力
2019/08/12 08:45:26 {1 true John Doe 65535 0.009876}

NullXX使ったプログラムの出力
2019/08/12 09:03:49 {1 {true true} {John Doe true} {65535 true} {0.009876 true}}

activeという列の出力内容が true ではなく {true true} となっている。
これは、NullBoolがBoolとValidという二つのフィールドに分かれているからである。
ValidはScanに成功したか否かを保持しており、実際の値はBoolから取り出す。

なので、NullableStudentを使ってScanしてStudentに詰め替えるプログラムに書きかえる(以下)。

for rows.Next() {
    s := NullableStudent{}
    if err := rows.Scan(&s.ID, &s.Active, &s.Name, &s.Grade, &s.Score); err != nil {
        log.Fatal(err)
    }

    st := Student{
        ID: s.ID,
        Active: s.Active.Bool,
        Name: s.Name.String,
        Grade: int(s.Grade.Int64),
        Score: s.Score.Float64,
    }
    log.Println(st)

途中の log.Fatal() にはもはやScanのエラーは来ないのであそこでは異常終了しない。
また、Scanに失敗したフィールドにはゼロ値が入るので、出力結果は以下の通り、NULLだった場合はそのgoの型におけるゼロ値、NULLでなければテーブルに格納されている値が出力される。

2019/08/12 09:17:52 {1 true John Doe 65535 0.009876}
2019/08/12 09:17:52 {2 false  0 0}
2019/08/12 09:17:52 {3 false Akira Toriyama 0 100}

サンプルコードの全体はここに置きました。

https://github.com/oyakata/gooooolang/blob/master/scan_no_nazo/hello2/main.go


特に、NULL許可する列にNULLを入れずにテストしているとまさかScanに失敗しているとは気づかないので、これは知っておいた方が良いでしょう。


最後までお読みくださりありとうございました。

コードレビューのはなし

エンジニアをやっているとコードレビューをしたり受けたりすることが日常的ですが、レビューは人間関係に絡むのでお悩みの職場も多いのではないでしょうか。

そんな中、ぼやっと思ったことをここに書いておきます。

人間関係が浅いうちはレビューコメントにタグつけるといい

レビューコメントにタグつける運用あるじゃないですか。

[MUST] ループの中で毎度このオブジェクトを初期化すると初期化コストがかさむのでループの外で一度だけ初期化するようにしてください。


[WANT] 要素の数が事前に判っているので、詰め替え先のスライスはmakeに長さを指定した方が省エネなので良いと思います。


[ASK] 好みの問題なんですけど、初期化した後にフィールドに代入するんじゃなくて、初期化のときに一緒に渡せば良いのではないかと思いました。

こういうの、いちいちつけるの面倒くさいと思うんですけど、レビューア・レビューイ*1の人間関係が浅いうちは付けておいた方がいいと思いました。

なぜタグをつけた方がいいかというと、MUSTだったら対応しないとapproveもらえないだろうから対応する、でもWANT/ASKだったら忙しいから・自分の好みじゃないから無視するといったやる・やらないの判断がしやすくなるからです。

そもそも、指摘事項にぜんぶ対応しないとapproveがもらえないレビューは厳しいです。
レビューアに権力があって、レビューイが服従するみたいな関係だとレビューを受けたくなくなります。
レビューアのコードの書き方の好みまで合わせなければならないというのはヤバイと思います。
だからレビューアはまず自分の指摘が絶対直して欲しいのか気が向いたら直して欲しいのか区別して指摘を書く必要があって、それを示すのにタグが役立ちます。

タグつけてレビューやってるのに直す・直さないで意見の一致が図れなくてレビューでもめたり、ストレス溜まるという人は、レビューやっても意味ないでしょうから、メンバーと喧嘩するなり、自分がチームを出て行くなり、相手を追放するなり好きにやってください。
多分その人たちは反りが合わないんでしょう。管理職に相談した方がいいかもしれません。

人間関係が浅いうちを卒業するのはいつなのかというと難しいんですけど、同じチームで一緒に仕事し始めて3ヶ月以内とかだったら互いに気ぃ遣い合った方がいいんじゃないでしょうか。

Githubのプルリクのresolve機能は使った方がいい

指摘を出してレビューイがプログラム直したら再レビューしますけど、レビューアは指摘したことに対する修正内容に満足したらresolveボタンを押して、その話が決着したことを明らかにしてあげると良いです。

同様に、レビュー受ける側も「ここは解決済みだ」と思ったらresolve押してしまって構わないと思います。

レビューアもレビューイもお互い適当にresolve機能使ったらいいと思います。せっかく用意されてるので。

「勝手にresolveするなよ」と互いにもめるようなら、話し合うなり何なりしてトラブらないよう努力してください。
チーム開発なんて互いに気持ちよく働いていく努力しないと成立しません。

間違いの訂正なのかリファクタなのか判るコメントを書いた方がいい

レビューアはプルリクを見ていて間違った修正をしていたら訂正を求めるコメントを出します。
そういう間違いの訂正がコードレビューの主な仕事になっていくと思います。
レビュー受けてる方も基本は指摘をもらったら自分の修正内容ないし方法が間違っているんだなと思います。

でも、ついでにリファクタしてほしいときもありますよね*2
レビューイにリファクタさせたいときは「これは間違いではなくて、リファクタを依頼している」と判るようコメントを書くとお互い捗ります。
さっき書いた「レビューコメントにタグをつける」とかはその辺にも役立ちます。

さいごに まず良い職場を確保した方がいい

コードの書き方でもめる職場はツラいので、まず気持ちよく働ける同僚が入ってくれる良い職場を作りましょう。
まともにレビューが成立しない人がなぜか同じチームに入ってくるという場合、まずその状況に問題意識を持ちましょう。

逆に、自分が知らない職場に入って行く場合、一緒に働く人とうまくやっていけるかよく考えて職場を選びましょう。
たぶん、自分よりスキルが上の人たちがいる職場に行くのはあまり問題にならないでしょうが、誤って自分より程度の低い人がいる職場に行ってしまうと本当に後悔します。
程度の悪い人とは働かないのが一番だとわたしは思います。

ちゃんとしたチームがあってこそ良いレビューとは何かという問題意識が生まれます。
問題の優先順位を履き違えないよう気をつけないといけません。


最後までお読みくださりありがとうございました。

*1:レビューで指摘する方がレビューアで、レビュー依頼を出して指摘受けて直す方がレビューイです。

*2:ひとつのプルリクで機能追加とリファクタを両方やって良いのか? などの論点はとりあえず置いておきます

Goのnilのmapやスライスで許されない操作

Goのmapやスライスを操作するコードを書いていて、やたらとレビュー指摘をもらったのでここにダメな操作と大丈夫な操作をメモしておく。

例えば、こういうコードは余計なことをしているので直しなさいという指摘をもらった。

package main

import "fmt"

func main() {
	m := map[string][]int{}

	xs, ok := m["foo"]
	if !ok {
		xs = []int{} // nilのスライスが取れるから初期化した方がいいんじゃないか・・?
	}
	xs = append(xs, 10)
	m["foo"] = xs

	fmt.Println(m)
}

これは、こう書けるらしい。

package main

import "fmt"

func main() {
	m := map[string][]int{}
	m["foo"] = append(m["foo"], 10)
	fmt.Println(m)
}

結論から言うと、nilのmapにキー渡して値を入れようとするとpanicになるのでダメ。これ以外は大丈夫。

操作 ok/NG 結果
nilのmapをlenに渡す
例: len(m)
ok 0を返す
nilのmapをrangeで回す
例: for k, v := range m { ... }
ok 一度もループが回らない
nilのmapからdeleteで要素を消す
例: delete(m, "foo")
ok 特に何も起きない
nilのmapにキー渡して値を入れる
例: m["foo"] = 1
NG panicになる
(panic: assignment to entry in nil map)
nilのスライスをlenやcapに渡す
例: len(xs), cap(xs)
ok 0を返す
nilのスライスをrangeで回す
例: or _, x := range xs { ... }
ok 一度もループが回らない
nilのスライスをappendで書き換える
例: xs = append(xs, 1)
ok 要素を追加できる

Playgroundで試したサンプルコード
https://play.golang.org/p/Kje5KtMOMOw

package main

import (
	"fmt"
)

func main() {
	var (
		m  map[string][]int = nil
		xs []int            = nil
	)

	fmt.Println("***** len, cap *****", len(xs), cap(xs)) // これは大丈夫: ***** len, cap ***** 0 0
	for _, x := range xs {
		fmt.Println("*****", x) // 1回も通らない: でもrangeに渡されてもエラーにはならない
	}

	fmt.Println("===== len =====", len(m)) // これも大丈夫: ===== len ===== 0
	for k, v := range m {
		fmt.Println("=====", k, v) // こっちも1回も通らない: エラーにはならない
	}

	delete(m, "par?") // nilのmapをdeleteに渡してもpanicにならない

	/*

		m["foo"] = []int{1, 2, 3} // これはダメ: panic: assignment to entry in nil map
		fmt.Println("m->", m)

	*/

	xs = append(xs, 10)     // これは通る
	fmt.Println("xs->", xs) // xs-> [10]
}