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: add TestCreateManyUnappliedProbes #104401

Merged
merged 8 commits into from
Jun 20, 2023
Merged

kvserver: add TestCreateManyUnappliedProbes #104401

merged 8 commits into from
Jun 20, 2023

Conversation

tbg
Copy link
Member

@tbg tbg commented Jun 6, 2023 •
edited
Loading

This is the test used for #102953.

This PR also makes modest progress on #75729, not by making log application
stand-alone, but by making it somewhat less convoluted to manufacture log
entries programmatically. It would be desirable to be able to test the
replication layer in CRDB in the same way that Raft allows via the
InteractionEnv1.

Epic: none
Release note: none

Footnotes

  1. https://github.com/etcd-io/raft/blob/6bf4f7fe3122b064e0a0d76314298dca6f379fc7/interaction_test.go ↩

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@tbg tbg requested a review from pav-kv June 6, 2023 10:44
@tbg tbg marked this pull request as ready for review June 6, 2023 10:45
@tbg tbg requested a review from a team June 6, 2023 10:45
@tbg tbg requested a review from a team as a code owner June 6, 2023 10:45
pav-kv
pav-kv approved these changes Jun 6, 2023
View reviewed changes
pkg/kv/kvserver/replica_raft.go Outdated Show resolved Hide resolved
pkg/kv/kvserver/replica_raft.go Outdated Show resolved Hide resolved
pkg/kv/kvserver/replica_raft.go Outdated Show resolved Hide resolved
pkg/kv/kvserver/replica_raft.go Outdated Show resolved Hide resolved
pkg/kv/kvserver/client_manual_proposal_test.go Outdated Show resolved Hide resolved
pkg/kv/kvserver/client_manual_proposal_test.go Outdated Show resolved Hide resolved
pkg/kv/kvserver/client_manual_proposal_test.go Outdated Show resolved Hide resolved
@tbg
Copy link
Member Author

tbg commented Jun 12, 2023 •
edited
Loading

Hold off on re-review, I want to first rebase on top of the replication admission control change.

@tbg tbg force-pushed the raft-log-gen branch 3 times, most recently from 296a7d9 to cb7b7b7 Compare June 12, 2023 11:22
@tbg tbg requested a review from pav-kv June 12, 2023 11:23
@tbg
Copy link
Member Author

tbg commented Jun 12, 2023

I had to basically re-do this PR as part of the rebase. I took the opportunity to rearrange the commits somewhat, and to bake in the renames we decided on earlier. Even though the structure follows very closely to the original work, please give it an honest review. (I self-reviewed already but one is usually blind to one's own slip-ups).
Thanks!

pav-kv
pav-kv approved these changes Jun 19, 2023
View reviewed changes
pkg/kv/kvserver/replica_raft.go Show resolved Hide resolved
pav-kv
pav-kv reviewed Jun 19, 2023
View reviewed changes
pkg/kv/kvserver/client_manual_proposal_test.go Outdated

// End of evaluation. Start of "proposing".

// TODO: the "requires consensus" logic is not reusable, make it so:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there an issue? More generally, is this test going to be unskipped at some point in some other form? Do we need to avoid this test being dead code?

If this test is intended as a demo, link it in the reverse direction from the issue that it serves? So that it's not forgotten.

Copy link
Member Author

Choose a reason for hiding this comment

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

Filed #105177 and cross-referring from the comments to that issue and back.

@tbg tbg mentioned this pull request Jun 20, 2023
Open
tbg added 8 commits June 20, 2023 15:03
@tbg
kvserver: move and rename makeIDKey()
2175fe2 
Epic: none
Release note: none
@tbg
raftlog: rename EncodeRaftCommand -> EncodeCommandBytes
42d9987 
That way, we can then rename RaftCmdToPayload to EncodeCommand without creating
confusion.

EncodeRaftCommand is only used in unquiesce (and testing).

Epic: none
Release note: None
@tbg
kvserver: move some logging in anticipation of a refactor
e88fb92 
Epic: none

Release note: None
@tbg
kvserver: begin extracting raftCmdToPayload
251f6f7 
Epic: none
Release note: None
@tbg
kvserver: extract raftCmdToPayload
a101704 
Epic: none
Release note: None
@tbg
kvserver: move change replicas validation out of raftCmdToPayload
41c4164 
Epic: none
Release note: none
@tbg
kvserver: move raftCmdToPayload to raftlog
65e0295 
Epic: none
Release note: none
@tbg
kvserver: add TestCreateManyUnappliedProbes
9fc9a65 
This is the test used for cockroachdb#102953.

Epic: none
Release note: none
@tbg
Copy link
Member Author

tbg commented Jun 20, 2023

bors r=pavelkalinnikov

@craig
Copy link
Contributor

craig bot commented Jun 20, 2023

Build succeeded:

@craig craig bot merged commit 2294fa8 into cockroachdb:master Jun 20, 2023
@erikgrinaker erikgrinaker mentioned this pull request Jun 21, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pav-kv pav-kv pav-kv approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@tbg @cockroach-teamcity @pav-kv