{"action":"created","issue":{"url":"https://api.github.com/repos/cockroachdb/cockroach/issues/8967","repository_url":"https://api.github.com/repos/cockroachdb/cockroach","labels_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/8967/labels{/name}","comments_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/8967/comments","events_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/8967/events","html_url":"https://github.com/cockroachdb/cockroach/pull/8967","id":174121128,"number":8967,"title":"sqlbase: use ResumeSpan in kvfetcher","user":{"login":"vivekmenezes","id":11774212,"avatar_url":"https://avatars.githubusercontent.com/u/11774212?v=3","gravatar_id":"","url":"https://api.github.com/users/vivekmenezes","html_url":"https://github.com/vivekmenezes","followers_url":"https://api.github.com/users/vivekmenezes/followers","following_url":"https://api.github.com/users/vivekmenezes/following{/other_user}","gists_url":"https://api.github.com/users/vivekmenezes/gists{/gist_id}","starred_url":"https://api.github.com/users/vivekmenezes/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/vivekmenezes/subscriptions","organizations_url":"https://api.github.com/users/vivekmenezes/orgs","repos_url":"https://api.github.com/users/vivekmenezes/repos","events_url":"https://api.github.com/users/vivekmenezes/events{/privacy}","received_events_url":"https://api.github.com/users/vivekmenezes/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":{"login":"RaduBerinde","id":16544120,"avatar_url":"https://avatars.githubusercontent.com/u/16544120?v=3","gravatar_id":"","url":"https://api.github.com/users/RaduBerinde","html_url":"https://github.com/RaduBerinde","followers_url":"https://api.github.com/users/RaduBerinde/followers","following_url":"https://api.github.com/users/RaduBerinde/following{/other_user}","gists_url":"https://api.github.com/users/RaduBerinde/gists{/gist_id}","starred_url":"https://api.github.com/users/RaduBerinde/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/RaduBerinde/subscriptions","organizations_url":"https://api.github.com/users/RaduBerinde/orgs","repos_url":"https://api.github.com/users/RaduBerinde/repos","events_url":"https://api.github.com/users/RaduBerinde/events{/privacy}","received_events_url":"https://api.github.com/users/RaduBerinde/received_events","type":"User","site_admin":false},"assignees":[{"login":"RaduBerinde","id":16544120,"avatar_url":"https://avatars.githubusercontent.com/u/16544120?v=3","gravatar_id":"","url":"https://api.github.com/users/RaduBerinde","html_url":"https://github.com/RaduBerinde","followers_url":"https://api.github.com/users/RaduBerinde/followers","following_url":"https://api.github.com/users/RaduBerinde/following{/other_user}","gists_url":"https://api.github.com/users/RaduBerinde/gists{/gist_id}","starred_url":"https://api.github.com/users/RaduBerinde/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/RaduBerinde/subscriptions","organizations_url":"https://api.github.com/users/RaduBerinde/orgs","repos_url":"https://api.github.com/users/RaduBerinde/repos","events_url":"https://api.github.com/users/RaduBerinde/events{/privacy}","received_events_url":"https://api.github.com/users/RaduBerinde/received_events","type":"User","site_admin":false}],"milestone":null,"comments":4,"created_at":"2016-08-30T20:43:59Z","updated_at":"2016-09-01T16:26:47Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/cockroachdb/cockroach/pulls/8967","html_url":"https://github.com/cockroachdb/cockroach/pull/8967","diff_url":"https://github.com/cockroachdb/cockroach/pull/8967.diff","patch_url":"https://github.com/cockroachdb/cockroach/pull/8967.patch"},"body":"Also ensure that the fetcher is not reading more than\r\none partial response.\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/8967)\n<!-- Reviewable:end -->\n"},"comment":{"url":"https://api.github.com/repos/cockroachdb/cockroach/issues/comments/244134287","html_url":"https://github.com/cockroachdb/cockroach/pull/8967#issuecomment-244134287","issue_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/8967","id":244134287,"user":{"login":"vivekmenezes","id":11774212,"avatar_url":"https://avatars.githubusercontent.com/u/11774212?v=3","gravatar_id":"","url":"https://api.github.com/users/vivekmenezes","html_url":"https://github.com/vivekmenezes","followers_url":"https://api.github.com/users/vivekmenezes/followers","following_url":"https://api.github.com/users/vivekmenezes/following{/other_user}","gists_url":"https://api.github.com/users/vivekmenezes/gists{/gist_id}","starred_url":"https://api.github.com/users/vivekmenezes/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/vivekmenezes/subscriptions","organizations_url":"https://api.github.com/users/vivekmenezes/orgs","repos_url":"https://api.github.com/users/vivekmenezes/repos","events_url":"https://api.github.com/users/vivekmenezes/events{/privacy}","received_events_url":"https://api.github.com/users/vivekmenezes/received_events","type":"User","site_admin":false},"created_at":"2016-09-01T16:26:47Z","updated_at":"2016-09-01T16:26:47Z","body":"\n\n\n\nReview status: 0 of 1 files reviewed at latest revision, 3 unresolved discussions, some commit checks pending.\n\n---\n\n*[sql/sqlbase/kvfetcher.go, line 148 \\[r1\\]](https://reviewable.io:443/reviews/cockroachdb/cockroach/8967#-KQW5TVw4u__QWvx4BVm:-KQaRyoB963SazROaeQo:b-t9olev) ([raw file](https://github.com/cockroachdb/cockroach/blob/7db038ba109244cb28493e7da5e789dad0d6b875/sql/sqlbase/kvfetcher.go#L148)):*\n<details><summary><i>Previously, RaduBerinde wrote…</i></summary>\n> We need to explain the requirements this function has on `spans` in the comment.\n> \n> Are we assuming that if `reverse` is true, then the spans are in increasing order? We should check that and panic if they are not.\n> \n> Now, the issue here is that there are cases (`indexJoin`) where `reverse` is `false` and the spans are NOT ordered. The flip side is that in that case we don't need to set any batch limits in that case (each span is a row so we are inherently limiting the results by limiting the number of spans) but we should assert that we don't ever send a request with unordered spans and a limit.\n</details>\nI've added a comment specifying the ordered limitation on spans and how they should be specified. I've been unable to reproduce triggering the more than one partial response error. I think it's best that we create a separate issue to create a test that triggers that at the SQL layer based on \nTestMultiRangeBoundedBatchScanUnsortedOrder and fix it by removing the batch limit where necessary. For now a user will see the more than one partial response error.\n\n---\n\n*[sql/sqlbase/kvfetcher.go, line 198 \\[r1\\]](https://reviewable.io:443/reviews/cockroachdb/cockroach/8967#-KQSGcAkZ8iCNwAcrw8G:-KQaNEYNacTlY7TewNsI:b-w495hl) ([raw file](https://github.com/cockroachdb/cockroach/blob/7db038ba109244cb28493e7da5e789dad0d6b875/sql/sqlbase/kvfetcher.go#L198)):*\n<details><summary><i>Previously, RaduBerinde wrote…</i></summary>\n> This is supposed to never happen right? We should do a `log.Errorf`\n</details>\nI've been unable to reproduce this error using existing tests and with kvBatchSize=2 . It certainly slows down the tests but there are no test failures. I've changed the error message to return the new resume spans.\n\n---\n\n*[sql/sqlbase/kvfetcher.go, line 212 \\[r1\\]](https://reviewable.io:443/reviews/cockroachdb/cockroach/8967#-KQW99Exmo5aFUeLoJJY:-KQaLrenL-cIS8K3ttEL:b-dvnbpp) ([raw file](https://github.com/cockroachdb/cockroach/blob/7db038ba109244cb28493e7da5e789dad0d6b875/sql/sqlbase/kvfetcher.go#L212)):*\n<details><summary><i>Previously, RaduBerinde wrote…</i></summary>\n> instead of using the`idx` array, could we reset `f.spans` before this loop and do something like `if results.ResumeSpan.Key != nil { f.spans = append(f.spans, result.ResumeSpan)}` here?\n</details>\nGood point! done\n\n---\n\n\n*Comments from [Reviewable](https://reviewable.io:443/reviews/cockroachdb/cockroach/8967)*\n<!-- Sent from Reviewable.io -->\n"}}
{"actor":{"display_login":"vivekmenezes"}}