{"action":"created","issue":{"url":"https://api.github.com/repos/cockroachdb/cockroach/issues/22737","repository_url":"https://api.github.com/repos/cockroachdb/cockroach","labels_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/22737/labels{/name}","comments_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/22737/comments","events_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/22737/events","html_url":"https://github.com/cockroachdb/cockroach/pull/22737","id":297516972,"number":22737,"title":"distsql: Check for cancellation while waiting for inbound streams","user":{"login":"abhimadan","id":7970201,"avatar_url":"https://avatars2.githubusercontent.com/u/7970201?v=4","gravatar_id":"","url":"https://api.github.com/users/abhimadan","html_url":"https://github.com/abhimadan","followers_url":"https://api.github.com/users/abhimadan/followers","following_url":"https://api.github.com/users/abhimadan/following{/other_user}","gists_url":"https://api.github.com/users/abhimadan/gists{/gist_id}","starred_url":"https://api.github.com/users/abhimadan/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/abhimadan/subscriptions","organizations_url":"https://api.github.com/users/abhimadan/orgs","repos_url":"https://api.github.com/users/abhimadan/repos","events_url":"https://api.github.com/users/abhimadan/events{/privacy}","received_events_url":"https://api.github.com/users/abhimadan/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":11,"created_at":"2018-02-15T16:47:20Z","updated_at":"2018-02-23T21:46:13Z","closed_at":null,"author_association":"MEMBER","pull_request":{"url":"https://api.github.com/repos/cockroachdb/cockroach/pulls/22737","html_url":"https://github.com/cockroachdb/cockroach/pull/22737","diff_url":"https://github.com/cockroachdb/cockroach/pull/22737.diff","patch_url":"https://github.com/cockroachdb/cockroach/pull/22737.patch"},"body":"Previously, the flow context was checked for cancellation just before\r\nthe server sent an RPC to producers. However, if the context was\r\ncanceled while blocked on the RPC, the cancellation wouldn't be picked\r\nup until the next producer RPC was sent.\r\n\r\nTo avoid this problem, the RPC is now sent in its own goroutine, and the\r\nserver waits for either context cancellation or the RPC to return,\r\nwhichever comes first.\r\n\r\nRelease note (performance improvement): Actively push cancellations up\r\nDistSQL flows, without waiting for the first row."},"comment":{"url":"https://api.github.com/repos/cockroachdb/cockroach/issues/comments/368147612","html_url":"https://github.com/cockroachdb/cockroach/pull/22737#issuecomment-368147612","issue_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/22737","id":368147612,"user":{"login":"abhimadan","id":7970201,"avatar_url":"https://avatars2.githubusercontent.com/u/7970201?v=4","gravatar_id":"","url":"https://api.github.com/users/abhimadan","html_url":"https://github.com/abhimadan","followers_url":"https://api.github.com/users/abhimadan/followers","following_url":"https://api.github.com/users/abhimadan/following{/other_user}","gists_url":"https://api.github.com/users/abhimadan/gists{/gist_id}","starred_url":"https://api.github.com/users/abhimadan/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/abhimadan/subscriptions","organizations_url":"https://api.github.com/users/abhimadan/orgs","repos_url":"https://api.github.com/users/abhimadan/repos","events_url":"https://api.github.com/users/abhimadan/events{/privacy}","received_events_url":"https://api.github.com/users/abhimadan/received_events","type":"User","site_admin":false},"created_at":"2018-02-23T21:46:13Z","updated_at":"2018-02-23T21:46:13Z","author_association":"MEMBER","body":"Calling `Recv()` in another goroutine was my original implementation (it had a goroutine leak, but that was fixable). However, I talked to @asubiotto about it and he thought (please correct me if I'm misquoting you Alfonso) that it would be better to have an explicit cancel signal, to be more consistent with the intra-node `ConsumerClosed` message, which has no direct analogue in the FlowStream RPC aside from non-EOF errors, which is somewhat subtle (I didn't name this new API message `ConsumerClosed`, but I can change that to make the connection more clear). I also thought that it would be a bit more flexible to be able to send cancellation signals before the server handler exits (which might be useful for more complex cancellation behaviour in the future), but perhaps that flexibility is unnecessary.\n\nAlso, I noticed that the `Recv()` would have some overhead to send the `*ProducerMessage` and `error` to the main goroutine (though I don't know if that overhead is significant). On the other hand, this version doesn't have much channel overhead.\n\nIt would require a lot less code to get this working if `Recv` has its own goroutine though. Overall I'm not too committed to either approach, and I don't mind switching back to put `Recv` in its own goroutine if we decide that's a better approach.\n\n---\n\nReview status: 0 of 4 files reviewed at latest revision, 6 unresolved discussions, all commit checks successful.\n\n---\n\n*[pkg/sql/distsqlrun/inbound.go, line 92 at r3](https://beta.reviewable.io/reviews/cockroachdb/cockroach/22737#-L63H_Qn0k0q21Jq6orA:-L63Zavk8pfohD-ThYJE:b-896fix) ([raw file](https://github.com/cockroachdb/cockroach/blob/3fa515f0e8070d3a792d8e9b6b546df981837a7d/pkg/sql/distsqlrun/inbound.go#L92)):*\n<details><summary><i>Previously, andreimatei (Andrei Matei) wrote…</i></summary><blockquote>\n\nnit: either say \"If the stream...\", or move the comment inside the `if`.\n</blockquote></details>\n\nDone.\n\n---\n\n*[pkg/sql/distsqlrun/inbound.go, line 174 at r3](https://beta.reviewable.io/reviews/cockroachdb/cockroach/22737#-L63HFy_4sGksyXjVfGQ:-L63ZdW_DTcPo5RpZAWa:b393omg) ([raw file](https://github.com/cockroachdb/cockroach/blob/3fa515f0e8070d3a792d8e9b6b546df981837a7d/pkg/sql/distsqlrun/inbound.go#L174)):*\n<details><summary><i>Previously, andreimatei (Andrei Matei) wrote…</i></summary><blockquote>\n\ncan this race with `sendDrainSignalToStreamProducer()`? Is that OK?\n</blockquote></details>\n\nYou're right, there is a race here, and gRPC doesn't like sending on the same stream from multiple goroutines so it's probably unsafe. I've locked both `sendDrainSignalToStreamProducer` and `sendCancelSignalToStreamProducer` to avoid this. Thanks for noticing this!\n\n---\n\n\n*Comments from [Reviewable](https://beta.reviewable.io/reviews/cockroachdb/cockroach/22737#-:-L63Z9ooDkBrOJX4A9Ep:b-alu31e)*\n<!-- Sent from Reviewable.io -->\n"}}
{"actor":{"display_login":"abhimadan"}}