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

kvserver: remove mtc from client_replica_test #59333

Merged

Conversation

lunevalex
Copy link
Collaborator

Makes progress on #8299

This commit removes the use of multiTestContect from client_replica_test
and replaces it with testcluster.TestCluster and server.TestServer. This
one change of many to completely remove multiTestContext from CRDB.

To replace these tests we introduce a new function on hlc.HybridManualClock
to pause the clock. This allows the test to conduct specific time based
measurements, which are otherwise tricky to do when the time is moving.

Release note: None

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@lunevalex lunevalex force-pushed the alex/replace-mtc-client-replica-test branch 3 times, most recently from b5bc2a4 to 3e93038 Compare January 25, 2021 03:16
Copy link
Member

@nvanbenschoten nvanbenschoten left a comment

Choose a reason for hiding this comment

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

:lgtm: one step closer!

Reviewed 4 of 4 files at r1.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @lunevalex and @tbg)


pkg/kv/kvserver/client_replica_test.go, line 129 at r1 (raw file):

		// application is asynchronous on all nodes.
		if write {
			testutils.SucceedsSoon(t, func() error {

Can we use tc.WaitForValues(t, reqKey, []int64{5, 5, 5}) here as well?


pkg/kv/kvserver/client_replica_test.go, line 608 at r1 (raw file):

			},
		},
		// Test the start

nit: punctuation


pkg/kv/kvserver/client_replica_test.go, line 1216 at r1 (raw file):

	splitDesc := tc.LookupRangeOrFatal(t, epochKey)
	err := tc.TransferRangeLease(splitDesc, tc.Target(1))
	// we expect this to fail

nit: capitalization and punctuation


pkg/kv/kvserver/client_replica_test.go, line 1228 at r1 (raw file):

	// Expire current leases and put a key to the epoch based scratch range to
	// get a lease. .

nit: strange extra period.


pkg/kv/kvserver/client_replica_test.go, line 2136 at r1 (raw file):

	})
	defer tc.Stopper().Stop(context.Background())
	store := tc.GetFirstStoreFromServer(t, 0)

nit: consider s/store/store0/


pkg/util/hlc/hlc.go, line 130 at r1 (raw file):

// creating a hybrid logical clock whose physical clock
// ticks with the wall clock, but that can be moved arbitrarily
// into the future. HybridManualClock is thread safe.

"or paused"


pkg/util/hlc/hlc.go, line 149 at r1 (raw file):

	nanosAtPause := atomic.LoadInt64(&m.nanosAtPause)
	if nanosAtPause > 0 {
		return atomic.LoadInt64(&m.nanos) + nanosAtPause

nit: consider restructuring this to avoid atomic.LoadInt64(&m.nanos) in two separate places.


pkg/util/hlc/hlc.go, line 161 at r1 (raw file):

// Pause pauses the hybrid manual clock and forces it to always return the
// current timestamp.
func (m *HybridManualClock) Pause() {

Do we need a way to unpause the clock as well?

Copy link
Collaborator Author

@lunevalex lunevalex left a comment

Choose a reason for hiding this comment

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

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


pkg/kv/kvserver/client_replica_test.go, line 129 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Can we use tc.WaitForValues(t, reqKey, []int64{5, 5, 5}) here as well?

Unfortunately not, the MVCCGet needs to pass in the reqTS, otherwise we dont find anything.


pkg/util/hlc/hlc.go, line 161 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Do we need a way to unpause the clock as well?

I havent needed it for any test yet, so I was hesitant to implement it until its needed.

@lunevalex lunevalex force-pushed the alex/replace-mtc-client-replica-test branch from 3e93038 to 54fe48c Compare January 25, 2021 17:32
Copy link
Member

@nvanbenschoten nvanbenschoten left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 2 of 2 files at r2.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @tbg)

Makes progress on cockroachdb#8299

This commit removes the use of multiTestContect from client_replica_test
and replaces it with testcluster.TestCluster and server.TestServer. This
one change of many to completely remove multiTestContext from CRDB.

To replace these tests we introduce a new function on hlc.HybridManualClock
to pause the clock. This allows the test to conduct specific time based
measurements, which are otherwise tricky to do when the time is moving.

Lastly, multiTestContext.restart() is removed to appease the lint gods,
since it's not used anywhere any more.

Release note: None
@lunevalex lunevalex force-pushed the alex/replace-mtc-client-replica-test branch from 54fe48c to ec416c9 Compare January 25, 2021 19:00
@lunevalex
Copy link
Collaborator Author

TFTR!

bors r=nvanbenschoten

@craig
Copy link
Contributor

craig bot commented Jan 25, 2021

Build succeeded:

@craig craig bot merged commit 0b1284f into cockroachdb:master Jan 25, 2021
erikgrinaker pushed a commit to erikgrinaker/cockroach that referenced this pull request Apr 29, 2021
This backports the `HybridManualClock` changes from cockroachdb#59333, but not the
client replica test changes which have additional backport dependencies.

---

We introduce a new function on hlc.HybridManualClock to pause the clock.
This allows a test to conduct specific time based measurements, which
are otherwise tricky to do when the time is moving.

Release note: None
erikgrinaker pushed a commit to erikgrinaker/cockroach that referenced this pull request Apr 29, 2021
This backports the `HybridManualClock` changes from cockroachdb#59333, but not the
client replica test changes which have additional backport dependencies.

---

We introduce a new function on hlc.HybridManualClock to pause the clock.
This allows a test to conduct specific time based measurements, which
are otherwise tricky to do when the time is moving.

Release note: None
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.

None yet

3 participants