{"action":"created","issue":{"url":"https://api.github.com/repos/cockroachdb/cockroach/issues/8865","repository_url":"https://api.github.com/repos/cockroachdb/cockroach","labels_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/8865/labels{/name}","comments_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/8865/comments","events_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/8865/events","html_url":"https://github.com/cockroachdb/cockroach/pull/8865","id":173579852,"number":8865,"title":"kv: introduce RacePromotingTransport, fix races","user":{"login":"tschottdorf","id":5076964,"avatar_url":"https://avatars.githubusercontent.com/u/5076964?v=3","gravatar_id":"","url":"https://api.github.com/users/tschottdorf","html_url":"https://github.com/tschottdorf","followers_url":"https://api.github.com/users/tschottdorf/followers","following_url":"https://api.github.com/users/tschottdorf/following{/other_user}","gists_url":"https://api.github.com/users/tschottdorf/gists{/gist_id}","starred_url":"https://api.github.com/users/tschottdorf/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/tschottdorf/subscriptions","organizations_url":"https://api.github.com/users/tschottdorf/orgs","repos_url":"https://api.github.com/users/tschottdorf/repos","events_url":"https://api.github.com/users/tschottdorf/events{/privacy}","received_events_url":"https://api.github.com/users/tschottdorf/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":2,"created_at":"2016-08-27T05:45:00Z","updated_at":"2016-08-27T15:25:04Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/cockroachdb/cockroach/pulls/8865","html_url":"https://github.com/cockroachdb/cockroach/pull/8865","diff_url":"https://github.com/cockroachdb/cockroach/pull/8865.diff","patch_url":"https://github.com/cockroachdb/cockroach/pull/8865.patch"},"body":"A BatchRequest may be passed on to multiple goroutines within the kv.Transport,\r\nand so effectively `DistSender` and `TxnCoordSender` should not assume that\r\nthey have exclusive access to any of the pointer fields in it. This was not\r\nalways obeyed and various data races were fixed in this commit.\r\n\r\nThey were identified with the newly introduced `RacePromotingTransport`, which\r\nintercepts all BatchRequests and continuously accesses them, vastly improving\r\nrace detector coverage in our `kv` unit tests.\r\n\r\nThe newly introduced test produced ~65 data races (from a handful of locations)\r\nwhich were all related to updating a `Transaction` proto without copy-on-write;\r\nmanually running *all* `kv` tests with the new transport exposed another one in\r\n`MergeSpans`.\r\n\r\nFixes #8020.\n\n<!-- Reviewable:start -->\n---\nThis change is [<img src=\"https://reviewable.io/review_button.svg\" height=\"34\" align=\"absmiddle\" alt=\"Reviewable\"/>](https://reviewable.io/reviews/cockroachdb/cockroach/8865)\n<!-- Reviewable:end -->\n"},"comment":{"url":"https://api.github.com/repos/cockroachdb/cockroach/issues/comments/242923393","html_url":"https://github.com/cockroachdb/cockroach/pull/8865#issuecomment-242923393","issue_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/8865","id":242923393,"user":{"login":"tschottdorf","id":5076964,"avatar_url":"https://avatars.githubusercontent.com/u/5076964?v=3","gravatar_id":"","url":"https://api.github.com/users/tschottdorf","html_url":"https://github.com/tschottdorf","followers_url":"https://api.github.com/users/tschottdorf/followers","following_url":"https://api.github.com/users/tschottdorf/following{/other_user}","gists_url":"https://api.github.com/users/tschottdorf/gists{/gist_id}","starred_url":"https://api.github.com/users/tschottdorf/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/tschottdorf/subscriptions","organizations_url":"https://api.github.com/users/tschottdorf/orgs","repos_url":"https://api.github.com/users/tschottdorf/repos","events_url":"https://api.github.com/users/tschottdorf/events{/privacy}","received_events_url":"https://api.github.com/users/tschottdorf/received_events","type":"User","site_admin":false},"created_at":"2016-08-27T15:25:04Z","updated_at":"2016-08-27T15:25:04Z","body":"\n\n\n\nReview status: all files reviewed at latest revision, 6 unresolved discussions, all commit checks successful.\n\n---\n\n*[kv/transport.go, line 298 \\[r1\\]](https://reviewable.io:443/reviews/cockroachdb/cockroach/8865#-KQAUhppAhFpP26A0jIx:-KQBaZxkbwL2gGuqRPBJ:bye5kwa) ([raw file](https://github.com/cockroachdb/cockroach/blob/ca9c15abedac731be7cbe44e1b1b1dddbf760c1e/kv/transport.go#L298)):*\n<details><summary><i>Previously, tschottdorf (Tobias Schottdorf) wrote…</i></summary>\n> `reflect.DeepEqual` would quit very early on (since it's... not equal). I could speculate that `reflect.DeepEqual(ba, ba)` would do 100% of the work, though, but  I'd rather stick with something that's more obviously correct.\n> \n> I'll have to play with speeding it up. I think the transport could be made more efficient if it had a single worker (i.e. the more pending requests, the less iterations per request). Will take a look at that option.\n</details>\n(btw, for most individual tests the slowdown isn't that bad, so this is really only an issue if we want to enable this for all tests in race mode)\n\n---\n\n\n*Comments from [Reviewable](https://reviewable.io:443/reviews/cockroachdb/cockroach/8865)*\n<!-- Sent from Reviewable.io -->\n"}}
{"actor":{"display_login":"tschottdorf"}}