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.

Sign up for GitHub

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

Jump to bottom

kvserver: migrate multiTestContext to TestCluster #8299

Closed
bdarnell opened this issue Aug 3, 2016 · 3 comments · Fixed by #59670 or #61561
Closed

kvserver: migrate multiTestContext to TestCluster #8299

bdarnell opened this issue Aug 3, 2016 · 3 comments · Fixed by #59670 or #61561
Labels
A-kv-replication Relating to Raft, consensus, and coordination. A-kv-server Relating to the KV-level RPC server A-testing Testing tools and infrastructure C-cleanup Tech debt, refactors, loose ends, etc. Solution not expected to significantly change behavior.

Comments

@bdarnell
Copy link
Contributor

bdarnell commented Aug 3, 2016

multiTestContext runs multiple Stores without Servers, and to do so it has to mock out the transport layer. Originally this was done because it was not feasible to use the regular Gossip and DistSender in this environment, but now we've moved closer to the standard environment and the last piece missing is the Server. We should go the rest of the way and move these tests to TestCluster, adding features to TestCluster as needed.

The immediate motivation for this change is deadlocks that result from multiTestContext using one RWMutex for too many things: #8170 and #7678.
@bdarnell bdarnell mentioned this issue Aug 3, 2016
Closed
@petermattis petermattis added this to the Later milestone Feb 22, 2017
@bdarnell bdarnell added C-cleanup Tech debt, refactors, loose ends, etc. Solution not expected to significantly change behavior. A-testing Testing tools and infrastructure labels Apr 26, 2018
@petermattis petermattis removed this from the Later milestone Oct 5, 2018
@tbg
Copy link
Member

tbg commented Dec 11, 2018

Sadly, this is still a problem (for the latest, see #32970) and we've started using multiTestContext in more places, usually in tests that need access to lots of internals (for example the merge tests).

@knz knz mentioned this issue Jun 8, 2020
Closed
@knz
Copy link
Contributor

knz commented Jun 8, 2020

We've seen an uptick of test failures in the past few weeks caused by a multiTestContext failing to shut down properly. Maybe time to revive this.

@knz knz added A-kv-replication Relating to Raft, consensus, and coordination. A-kv-server Relating to the KV-level RPC server labels Jun 8, 2020
@knz knz mentioned this issue Jun 12, 2020
Closed
@knz
Copy link
Contributor

knz commented Jun 12, 2020

cc @lunevalex we discussed this yesterday

@irfansharif irfansharif changed the title storage: migrate multiTestContext to TestCluster kvserver: migrate multiTestContext to TestCluster Jun 12, 2020
lunevalex added a commit to lunevalex/cockroach that referenced this issue Jun 16, 2020
@lunevalex
kv: Replace multiTestContext with TestCluster in consistency_queue_test
b3aaaa7 
Makes progress on cockroachdb#8299

multiTestContext is legacy construct that is deprecated in favor of running tests via TestCluster. This is one PR out of many to remove the usage of multiTestContext in the consistency_queue test cases.

Release note : None
@lunevalex lunevalex mentioned this issue Jun 16, 2020
Merged
lunevalex added a commit to lunevalex/cockroach that referenced this issue Jun 23, 2020
@lunevalex
kv: Replace multiTestContext with TestCluster in consistency_queue_test
7eeb6db 
Makes progress on cockroachdb#8299

multiTestContext is legacy construct that is deprecated in favor of running tests via TestCluster. This is one PR out of many to remove the usage of multiTestContext in the consistency_queue test cases.

Release note : None
@lunevalex lunevalex mentioned this issue Jun 25, 2020
Merged
lunevalex added a commit to lunevalex/cockroach that referenced this issue Jun 26, 2020
@lunevalex
kvserver: Replace multiTestContext with TestCluster in client_replica…
4438167 
…_gc_test

Makes progress on cockroachdb#8299

multiTestContext is legacy construct that is deprecated in favor of running
tests via TestCluster. This is one PR out of many to remove the usage of
multiTestContext in the client_replica_gc test cases.

Release note: none
lunevalex added a commit to lunevalex/cockroach that referenced this issue Jun 26, 2020
@lunevalex
kv: Replace multiTestContext with TestCluster in consistency_queue_test
59306ed 
Makes progress on cockroachdb#8299

multiTestContext is legacy construct that is deprecated in favor of running tests via TestCluster. This is one PR out of many to remove the usage of multiTestContext in the consistency_queue test cases.

Release note : None
craig bot pushed a commit that referenced this issue Jun 27, 2020
@lunevalex
Merge #50257
d8b3058 
50257: kv: Replace multiTestContext with TestCluster in consistency_queue_test r=lunevalex a=lunevalex

Makes progress on #8299

multiTestContext is legacy construct that is deprecated in favor of running tests via TestCluster. This is one PR out of many to remove the usage of multiTestContext in the consistency_queue test cases.

Release note : None

Co-authored-by: Alex Lunev <[email protected]>
lunevalex added a commit to lunevalex/cockroach that referenced this issue Jun 29, 2020
@lunevalex
kvserver: Replace multiTestContext with TestCluster in client_replica…
b43124c 
…_gc_test

Makes progress on cockroachdb#8299

multiTestContext is legacy construct that is deprecated in favor of running
tests via TestCluster. This is one PR out of many to remove the usage of
multiTestContext in the client_replica_gc test cases.

Release note: none
lunevalex added a commit to lunevalex/cockroach that referenced this issue Feb 3, 2021
@lunevalex
kvserver: convert the last set of multiTestContext tests to use TestC…
234d1c1 
…luster

Makes progress on cockroachdb#8299

This change converts the last set of tests in client_metrics_test,
client_raft_test, client_raft_log_queue_test to use TestCluster instead
of multiTestContext.

Release note: None
lunevalex added a commit to lunevalex/cockroach that referenced this issue Feb 3, 2021
@lunevalex
kvserver: remove multiTestContext
dfd9c60 
Closes cockroachdb#8299

With all the tests converted to use TestCluster/TestServer,
we can finally remove multiTestContext and all of the dependent
code.

Release note: None
lunevalex added a commit to lunevalex/cockroach that referenced this issue Feb 5, 2021
@lunevalex
kvserver: convert the last set of multiTestContext tests to use TestC…
624cc58 
…luster

Makes progress on cockroachdb#8299

This change converts the last set of tests in client_metrics_test,
client_raft_test, client_raft_log_queue_test to use TestCluster instead
of multiTestContext.

Release note: None
lunevalex added a commit to lunevalex/cockroach that referenced this issue Feb 5, 2021
@lunevalex
kvserver: remove multiTestContext
fbdade8 
Closes cockroachdb#8299

With all the tests converted to use TestCluster/TestServer,
we can finally remove multiTestContext and all of the dependent
code.

Release note: None
lunevalex added a commit to lunevalex/cockroach that referenced this issue Feb 6, 2021
@lunevalex
kvserver: convert the last set of multiTestContext tests to use TestC…
dd8197f 
…luster

Makes progress on cockroachdb#8299

This change converts the last set of tests in client_metrics_test,
client_raft_test, client_raft_log_queue_test to use TestCluster instead
of multiTestContext.

Release note: None
@craig craig bot closed this as completed in 04428fc Feb 8, 2021
@lunevalex lunevalex mentioned this issue Mar 5, 2021
Merged
lunevalex added a commit to lunevalex/cockroach that referenced this issue Mar 5, 2021
@lunevalex
kvserver: remove multiTestContext
8560678 
Closes cockroachdb#8299

With all the tests converted to use TestCluster/TestServer,
we can finally remove multiTestContext and all of the dependent
code.

Release note: None
Release justification: Cleans up unused test code.
craig bot pushed a commit that referenced this issue Mar 8, 2021
@barryhe2000 @lunevalex
Merge #61540 #61561
e9592f5 
61540: sql: use stopper for virtual table row pushing go routine r=barryhe2000 a=barryhe2000

Previously, the row pushing go routine in virtual table
would not go through the stopper. Now it uses the stopper
to run an async task, allowing the stopper to help with
error handling.

Fixes: #60587 

Release justification: bug fix and low-risk update

Release note: None

61561: kvserver: remove multiTestContext r=lunevalex a=lunevalex

Closes #8299

With all the tests converted to use TestCluster/TestServer,
we can finally remove multiTestContext and all of the dependent
code.

Release note: None
Release justification: Cleans up unused test code.

Co-authored-by: Barry He <[email protected]>
Co-authored-by: Alex Lunev <[email protected]>
@lunevalex lunevalex mentioned this issue Mar 15, 2021
Closed
erikgrinaker pushed a commit to erikgrinaker/cockroach that referenced this issue Apr 29, 2021
@lunevalex @erikgrinaker
testcluster: Introduce a HybridManualClock and wire it into TestCluster
7b8b24b 
Makes progress on cockroachdb#8299

This commit introduces a new type of ManualClock a HybridManualClock.
and wires it into the TestCluster.  This clock follows the physical
wall time of a regular clock, but allows the developer to move it
forward. This is needed to be able to test functionality around
lease expiration and other time based mechanisms.

To verify that the clock is usefull in tests, a single test in
client_merge_tests.go is converted to use TestCluster with a
HybridManualClock. To make the test possible we also need a
simple way to create a range with an expiration based lease. This
is done through the new TestCluster.ScratchRangeWithExpirationLease
function.

Release note: None
erikgrinaker pushed a commit to erikgrinaker/cockroach that referenced this issue Apr 29, 2021
@lunevalex @erikgrinaker
testcluster: Introduce a HybridManualClock and wire it into TestCluster
f40b9c1 
Makes progress on cockroachdb#8299

This commit introduces a new type of ManualClock a HybridManualClock.
and wires it into the TestCluster.  This clock follows the physical
wall time of a regular clock, but allows the developer to move it
forward. This is needed to be able to test functionality around
lease expiration and other time based mechanisms.

To verify that the clock is usefull in tests, a single test in
client_merge_tests.go is converted to use TestCluster with a
HybridManualClock. To make the test possible we also need a
simple way to create a range with an expiration based lease. This
is done through the new TestCluster.ScratchRangeWithExpirationLease
function.

Release note: None
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-kv-replication Relating to Raft, consensus, and coordination. A-kv-server Relating to the KV-level RPC server A-testing Testing tools and infrastructure C-cleanup Tech debt, refactors, loose ends, etc. Solution not expected to significantly change behavior.
Projects
None yet
Milestone
No milestone
4 participants
@bdarnell @knz @tbg @petermattis