Skip to content
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

kv: Fix test to correctly test the SystemClass #83214

Merged
merged 1 commit into from
Jun 23, 2022

Conversation

andrewbaptist
Copy link
Collaborator

Previously this test had several issues that are all addressed.
First the system range it was writing to only had a single replica, so
even if it was correctly written to, a network partition wouldn't test
anything. Additionally, the address it was attempting to write to was
a lease change, which is not in the system range. The reason this worked
previously is that there was an early exist from the change lease code
if the old and new leaseholders were the same, so it didn't actually
attempt to write to the leaseholder name at all.

This change addresses all this, but writing to a key in the liveness
range and first replicating the liveness range to all nodes.

Release note: None

@andrewbaptist andrewbaptist requested a review from a team as a code owner June 22, 2022 18:54
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@andrewbaptist andrewbaptist requested a review from ajwerner June 22, 2022 18:54
@andrewbaptist andrewbaptist force-pushed the connection_disruption branch from 411edc2 to ba7b9bc Compare June 22, 2022 22:03
Copy link
Contributor

@ajwerner ajwerner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm: thanks for fixing the test!

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @andrewbaptist)


pkg/kv/kvserver/client_raft_test.go line 4684 at r2 (raw file):

	// disabled controls whether to disrupt DefaultClass streams.
	var disabled, disabledSystem atomic.Value

one thing that didn't exist when this was written was syncutil.AtomicBool


pkg/kv/kvserver/client_raft_test.go line 4763 at r2 (raw file):

		// Write to the liveness range on the System class.
		require.NoError(t, db.Put(withTimeout, keyLiveness, 2), "Expected success writing to liveness range")

nit: wrap these long lines

Code quote:

		require.ErrorIs(t, db.Put(withTimeout, keyLiveness, 2), context.DeadlineExceeded, "Expected timeout writing to liveness range")

Previously this test had several issues that are all addressed.
First the system range it was writing to only had a single replica, so
even if it was correctly written to, a network partition wouldn't test
anything. Additionally, the address it was attempting to write to was
a lease change, which is not in the system range. The reason this worked
previously is that there was an early exist from the change lease code
if the old and new leaseholders were the same, so it didn't actually
attempt to write to the leaseholder name at all.

This change addresses all this, but writing to a key in the liveness
range and first replicating the liveness range to all nodes.

Release note: None
@andrewbaptist andrewbaptist force-pushed the connection_disruption branch from ba7b9bc to 7c70b06 Compare June 23, 2022 12:40
@andrewbaptist
Copy link
Collaborator Author

pkg/kv/kvserver/client_raft_test.go line 4684 at r2 (raw file):

Previously, ajwerner wrote…

one thing that didn't exist when this was written was syncutil.AtomicBool

Is the primary advantage the removal of the need to cast? Not worth changing for this commit I think

@andrewbaptist
Copy link
Collaborator Author

pkg/kv/kvserver/client_raft_test.go line 4763 at r2 (raw file):

Previously, ajwerner wrote…

nit: wrap these long lines

Done

@andrewbaptist
Copy link
Collaborator Author

bors r=ajwerner

@craig
Copy link
Contributor

craig bot commented Jun 23, 2022

Build failed:

@andrewbaptist
Copy link
Collaborator Author

unrelated flake

bors r=ajwerner

@craig
Copy link
Contributor

craig bot commented Jun 23, 2022

Build succeeded:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants