roachpb: Change RangeDescriptor.StickyBit and ReplicaDescriptor.Type to nullable by jeffrey-xiao · Pull Request #38465 · cockroachdb/cockroach

jeffrey-xiao · 2019-06-26T20:45:14Z

On a mixed version cluster, only having #38302 does not guarantee backwards compatibility for non-nullable fields in RangeDescriptor. Suppose the replicas of a range are of mixed version and the current
leaseholder of the range is on newest version. If the leaseholder splits that range and writes to the non-nullable fields in RangeDescriptor and the leaseholder transfers to a node on an older version without #38302, then all CPuts would fail on that node.

We must guarantee that #38302 is on all nodes before making the fields in RangeDescriptor non-nullable.

Fixes #36983.
Fixes #38471.

Release note: None

cockroach-teamcity · 2019-06-26T20:45:21Z

This change is

nvb

Reviewed 12 of 13 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @danhhz, @jeffrey-xiao, and @nvanbenschoten)

pkg/storage/replica_command.go, line 188 at r1 (raw file):

	// it to hlc.Timestamp{}. This check ensures that CPuts would not fail on
	// older versions.
	stickyBitEnabled := r.store.ClusterSettings().Version.IsActive(cluster.VersionStickyBit)

We will only ever set the sticky bit on an AdminSplitRequest to a value that's not its zero value if this is already true, right? In that case, I think we should replace this with a check that the expiration time in the request is not the zero value. The reason for this is that nodes can learn that a cluster version is active at different times and we want to avoid the case where one node starts supplying an expiration value in its AdminSplitRequest, expecting this to be respected, while the node that evaluates the request (which hasn't yet learned about the new cluster version) is still dropping the values on the floor.

jeffrey-xiao

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @danhhz and @nvanbenschoten)

pkg/storage/replica_command.go, line 188 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

We will only ever set the sticky bit on an AdminSplitRequest to a value that's not its zero value if this is already true, right? In that case, I think we should replace this with a check that the expiration time in the request is not the zero value. The reason for this is that nodes can learn that a cluster version is active at different times and we want to avoid the case where one node starts supplying an expiration value in its AdminSplitRequest, expecting this to be respected, while the node that evaluates the request (which hasn't yet learned about the new cluster version) is still dropping the values on the floor.

Thanks for the catch!

nvb

Reviewed 1 of 1 files at r2, 7 of 8 files at r3.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @danhhz, @jeffrey-xiao, and @nvanbenschoten)

pkg/roachpb/metadata_replicas.go, line 49 at r3 (raw file):

	// Note that the wrapped replicas are sorted first by type.
	for i := range d.wrapped {
		if d.wrapped[i].Type != nil && *d.wrapped[i].Type == ReplicaType_LEARNER {

Do you think it would be worth creating and using a method like:

func (rt *ReplicaType) Deref() ReplicaType {
    if rt == nil {
        return ReplicaType_VOTER
    }
    return *rt
}

It would belong in pkg/roachpb/metadata.go if we did.

pkg/roachpb/metadata_replicas_test.go, line 26 at r3 (raw file):

	tests := [][]ReplicaDescriptor{
		{},
		{{Type: newReplicaType(ReplicaType_VOTER)}},

nit: I'd pull these two allocs up and use them in each test case. Something like:

func TestVotersLearnersAll(t *testing.T) {
    voter := newReplicaType(ReplicaType_VOTER)
    learner := newReplicaType(ReplicaType_LEARNER)
    ...

jeffrey-xiao

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @danhhz and @nvanbenschoten)

pkg/roachpb/metadata_replicas.go, line 49 at r3 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Do you think it would be worth creating and using a method like:
func (rt *ReplicaType) Deref() ReplicaType {
    if rt == nil {
        return ReplicaType_VOTER
    }
    return *rt
}
It would belong in pkg/roachpb/metadata.go if we did.

Done. I've added a method to ReplicaDescriptor similar to what we do with Generation for RangeDescriptor.

nvb

thanks for turning this around so quickly.

Reviewed 3 of 3 files at r4.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @danhhz)

jeffrey-xiao · 2019-06-26T23:08:16Z

Was inspired by the GetType change, so I also added GetStickyBit.

nvb

Still

Reviewed 7 of 14 files at r5, 1 of 1 files at r6, 8 of 8 files at r7.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @danhhz)

jeffrey-xiao · 2019-06-26T23:18:13Z

bors r+

38465: roachpb: Change RangeDescriptor.StickyBit and ReplicaDescriptor.Type to nullable r=jeffrey-xiao a=jeffrey-xiao On a mixed version cluster, only having #38302 does not guarantee backwards compatibility for non-nullable fields in RangeDescriptor. Suppose the replicas of a range are of mixed version and the current leaseholder of the range is on newest version. If the leaseholder splits that range and writes to the non-nullable fields in RangeDescriptor and the leaseholder transfers to a node on an older version without #38302, then all CPuts would fail on that node. We must guarantee that #38302 is on all nodes before making the fields in RangeDescriptor non-nullable. Fixes #36983. Fixes #38471. Release note: None Co-authored-by: Jeffrey Xiao <jeffrey.xiao1998@gmail.com>

craig · 2019-06-26T23:45:03Z

Build failed

GitHub CI (Cockroach)

danhhz

ReplicaDescriptor.Type stuff lgtm. Thanks for the fix!

Release note: None

jeffrey-xiao · 2019-06-27T04:57:55Z

Seems to be a flake in TeamCity?
bors r+

craig · 2019-06-27T05:11:47Z

Build failed

GitHub CI (Cockroach)

jeffrey-xiao · 2019-06-27T12:23:03Z

Flake: #38458, #38468

bors r+

38465: roachpb: Change RangeDescriptor.StickyBit and ReplicaDescriptor.Type to nullable r=jeffrey-xiao a=jeffrey-xiao On a mixed version cluster, only having #38302 does not guarantee backwards compatibility for non-nullable fields in RangeDescriptor. Suppose the replicas of a range are of mixed version and the current leaseholder of the range is on newest version. If the leaseholder splits that range and writes to the non-nullable fields in RangeDescriptor and the leaseholder transfers to a node on an older version without #38302, then all CPuts would fail on that node. We must guarantee that #38302 is on all nodes before making the fields in RangeDescriptor non-nullable. Fixes #36983. Fixes #38471. Release note: None Co-authored-by: Jeffrey Xiao <jeffrey.xiao1998@gmail.com>

craig · 2019-06-27T12:57:49Z

Build succeeded

GitHub CI (Cockroach)

@asubiotto

38593: storage: sort InternalReplicas before comparing r=jeffrey-xiao a=jeffrey-xiao Fixes an issue where the in-memory copy of RangeDescriptor is out-of-sync with the copy in kv. Should resolve the failure in this #36983 (comment) The import step of `tpcc/mixed-headroom/n5cpu16` passes on `2811e0f` + #38465 + this PR. cc @asubiotto Co-authored-by: Jeffrey Xiao <jeffrey.xiao1998@gmail.com>

84420: sql/logictests: don't fail parent test when using retry r=knz a=stevendanna testutils.SucceedsSoon calls Fatal() on the passed testing.T. Here, we were calling SucceedsSoon with the root T, which in the case of a subtest resulted in this error showing up in our logs testing.go:1169: test executed panic(nil) or runtime.Goexit: subtest may have called FailNow on a parent test This moves to using SucceedsSoonError so that we can process errors using the same formatting that we use elsewhere. Release note: None 85120: roachpb: make range/replica descriptor fields non-nullable r=pavelkalinnikov a=erikgrinaker This patch makes all fields in range/replica descriptors non-nullable, fixing a long-standing TODO. Additionally, this fixes a set of bugs where code would use regular comparison operators (e.g. `==`) to compare replica descriptors, which with nullable pointer fields would compare the memory locations rather than the values. One practical consequence of this was that the DistSender would fail to use a new leaseholder with a non-`VOTER` type (e.g. `VOTER_INCOMING`), instead continuing to try other replicas before backing off. However, there are further issues surrounding this bug and it will be addressed separately in a way that is more amenable to backporting. The preparatory work for this was done in ea720e3. Touches #85060. Touches #38308. Touches #38465. Release note: None 85352: opt: revert data race fix r=mgartner a=mgartner This commit reverts #37972. We no longer lazily build filter props and share them across multiple threads. Release note: None Co-authored-by: Steven Danna <danna@cockroachlabs.com> Co-authored-by: Erik Grinaker <grinaker@cockroachlabs.com> Co-authored-by: Marcus Gartner <marcus@cockroachlabs.com>

jeffrey-xiao force-pushed the nullable-sticky-bit branch from 54e022f to 2261df9 Compare June 26, 2019 21:11

jeffrey-xiao marked this pull request as ready for review June 26, 2019 21:53

jeffrey-xiao requested review from a team, danhhz and nvb June 26, 2019 21:53

nvb reviewed Jun 26, 2019

View reviewed changes

jeffrey-xiao changed the title ~~roachpb: Change StickyBit back to nullable~~ roachpb: Change RangeDescriptor.StickyBit and ReplicaDescriptor.Type to nullable Jun 26, 2019

jeffrey-xiao force-pushed the nullable-sticky-bit branch from 2261df9 to 6c53401 Compare June 26, 2019 22:29

jeffrey-xiao commented Jun 26, 2019

View reviewed changes

nvb reviewed Jun 26, 2019

View reviewed changes

jeffrey-xiao force-pushed the nullable-sticky-bit branch from 6c53401 to b5ffcc3 Compare June 26, 2019 22:54

jeffrey-xiao commented Jun 26, 2019

View reviewed changes

nvb approved these changes Jun 26, 2019

View reviewed changes

jeffrey-xiao force-pushed the nullable-sticky-bit branch from b5ffcc3 to 8fded96 Compare June 26, 2019 23:07

jeffrey-xiao force-pushed the nullable-sticky-bit branch from 8fded96 to 692a4fa Compare June 26, 2019 23:11

nvb approved these changes Jun 26, 2019

View reviewed changes

danhhz reviewed Jun 27, 2019

View reviewed changes

jeffrey-xiao force-pushed the nullable-sticky-bit branch 2 times, most recently from 80ac8aa to fa5fb05 Compare June 27, 2019 01:28

jeffrey-xiao added 2 commits June 27, 2019 00:03

roachpb: change RangeDescriptor.StickyBit to nullable

23e4926

Release note: None

roachpb: change ReplicaDescriptor.Type to nullable

76f5f1c

Release note: None

jeffrey-xiao force-pushed the nullable-sticky-bit branch from fa5fb05 to 76f5f1c Compare June 27, 2019 04:03

craig bot merged commit 76f5f1c into cockroachdb:master Jun 27, 2019

danhhz mentioned this pull request Jun 27, 2019

release: v19.2.0-alpha.20190701 #38381

Closed

14 tasks

jeffrey-xiao mentioned this pull request Jul 1, 2019

storage: sort InternalReplicas before comparing #38593

Merged

jeffrey-xiao deleted the nullable-sticky-bit branch August 15, 2019 17:48

erikgrinaker mentioned this pull request Jul 27, 2022

roachpb: make range/replica descriptor fields non-nullable #85120

Merged

Conversation

jeffrey-xiao commented Jun 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cockroach-teamcity commented Jun 26, 2019

Uh oh!

nvb left a comment

Choose a reason for hiding this comment

Uh oh!

jeffrey-xiao left a comment

Choose a reason for hiding this comment

Uh oh!

nvb left a comment

Choose a reason for hiding this comment

Uh oh!

jeffrey-xiao left a comment

Choose a reason for hiding this comment

Uh oh!

nvb left a comment

Choose a reason for hiding this comment

Uh oh!

jeffrey-xiao commented Jun 26, 2019

Uh oh!

nvb left a comment

Choose a reason for hiding this comment

Uh oh!

jeffrey-xiao commented Jun 26, 2019

Uh oh!

craig bot commented Jun 26, 2019

Build failed

Uh oh!

danhhz left a comment

Choose a reason for hiding this comment

Uh oh!

jeffrey-xiao commented Jun 27, 2019

Uh oh!

craig bot commented Jun 27, 2019

Build failed

Uh oh!

jeffrey-xiao commented Jun 27, 2019

Uh oh!

craig bot commented Jun 27, 2019

Build succeeded

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jeffrey-xiao commented Jun 26, 2019 •

edited

Loading