/cockroach

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

backport-2.1: kv: try next replica on RangeNotFoundError #31250

Merged
merged 5 commits into from Oct 12, 2018

Conversation

Reviewers

@bdarnell bdarnell

@cockroachdb/sql-rest-prs sql-rest-prs

@cockroachdb/sql-opt-prs sql-opt-prs

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@tschottdorf @cockroach-teamcity @bdarnell
@tschottdorf
Member

tschottdorf commented Oct 11, 2018

Backport 5/5 commits from #31013.

/cc @cockroachdb/release

Previously, if a Batch RPC came back with a RangeNotFoundError, we would
immediately stop trying to send to more replicas, evict the range
descriptor, and start a new attempt after a back-off.

This new attempt could end up using the same replica, so if the
RangeNotFoundError persisted for some amount of time, so would the
unsuccessful retries for requests to it as DistSender doesn't aggressively
shuffle the replicas.

It turns out that there are such situations, and the election-after-restart
roachtest spuriously hit one of them:

  1. new replica receives a preemptive snapshot and the ConfChange
  2. cluster restarts
  3. now the new replica is in this state until the range wakes
    up, which may not happen for some time. 4. the first request to the range
    runs into the above problem

@nvanbenschoten: I think there is an issue to be filed about the tendency
of DistSender to get stuck in unfortunate configurations.

Fixes #30613.

Release note (bug fix): Avoid repeatedly trying a replica that was found to
be in the process of being added.

@tschottdorf tschottdorf requested a review from bdarnell Oct 11, 2018

@tschottdorf tschottdorf requested review from cockroachdb/core-prs as code owners Oct 11, 2018

@cockroach-teamcity

This comment has been minimized.

Show comment
Hide comment
@cockroach-teamcity

cockroach-teamcity Oct 11, 2018

Member

This change is Reviewable
Member

cockroach-teamcity commented Oct 11, 2018

This change is Reviewable

@tschottdorf tschottdorf referenced this pull request Oct 11, 2018

Closed

storage: more proactively replicaGC replicas with stuck commands #26952

@bdarnell

bdarnell approved these changes Oct 11, 2018
View changes

Reviewed 1 of 1 files at r1, 8 of 10 files at r3, 1 of 3 files at r4, 6 of 6 files at r5.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained

tschottdorf added some commits Oct 3, 2018

@tschottdorf
roachtest: print output on failure in election test 
This was hiding the output if the invocation itself failed, which is
when you wanted it most.

Release note: None
629267e
@tschottdorf
kv: improve a trace event about descriptor eviction 
Release note: None
4d0fb44
@tschottdorf
storage: report optional StoreID with RangeNotFoundError 
Release note: None
9998c7a
@tschottdorf
kv: try next replica on RangeNotFoundError 
Previously, if a Batch RPC came back with a RangeNotFoundError,
we would immediately stop trying to send to more replicas, evict the
range descriptor, and start a new attempt after a back-off.

This new attempt could end up using the same replica, so if the
RangeNotFoundError persisted for some amount of time, so would the
unsuccessful retries for requests to it as DistSender doesn't
aggressively shuffle the replicas.

It turns out that there are such situations, and the
election-after-restart roachtest spuriously hit one of them:

1. new replica receives a preemptive snapshot and the ConfChange
2. cluster restarts
3. now the new replica is in this state until the range wakes
   up, which may not happen for some time.
4. the first request to the range runs into the above problem

@nvanbenschoten: I think there is an issue to be filed about the
tendency of DistSender to get stuck in unfortunate configurations.

Fixes #30613.

Release note (bug fix): Avoid repeatedly trying a replica that was found
to be in the process of being added.
6aef2e4
@tschottdorf
kv: cache leaseholder on successful response 
Whenever a successful response is received from an RPC that we know has
to contact the leaseholder to succeed, update the leaseholder cache.

The immediate motivation for this is to be able to land the preceding
commits, which greatly exacerbated (as in, added a much faster failure
mode to)

```
make stress PKG=./pkg/sql/logictest TESTS=TestPlannerLogic/5node-dist/distsql_interleaved_join
```

However, the change is one we've wanted to make for a while; our caching
and in particular the eviction of leaseholders has been deficient
essentially ever since it was first introduced.

Touches #31068.

Release note: None
Loading status checks…
fdca0bd

@tschottdorf tschottdorf merged commit a7a3cc1 into cockroachdb:release-2.1 Oct 12, 2018
2 checks passed

2 checks passed

GitHub CI (Cockroach) TeamCity build finished
Details
license/cla Contributor License Agreement is signed.
Details

@tschottdorf tschottdorf deleted the tschottdorf:backport2.1-31013 branch Oct 12, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment