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

replica_rac2: add admitted state protocol #130074

Merged
merged 4 commits into from
Sep 4, 2024

Conversation

pav-kv
Copy link
Collaborator

@pav-kv pav-kv commented Sep 4, 2024

This PR introduces AdmittedState and PiggybackedAdmittedState protos, and converts replica- and leader-side code (such as AdmittedPiggybacker and raft transport) to use them.

The protos are not populated at the moment, and the leader-side code is a bunch of TODOs. More conversion code will follow, to replace the AdmittedResponseForRange flows.

Part of #129508

@pav-kv pav-kv requested a review from sumeerbhola September 4, 2024 09:52
@pav-kv pav-kv requested review from a team as code owners September 4, 2024 09:52
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@pav-kv pav-kv force-pushed the admission-vector-proto branch 2 times, most recently from cf1e9f6 to 05e161c Compare September 4, 2024 10:22
Epic: none
Release note: none
@pav-kv pav-kv force-pushed the admission-vector-proto branch from 05e161c to af15b82 Compare September 4, 2024 10:27
@pav-kv pav-kv force-pushed the admission-vector-proto branch 3 times, most recently from a5535d9 to 58cfecb Compare September 4, 2024 12:06
Copy link
Collaborator

@sumeerbhola sumeerbhola 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 r1, 1 of 1 files at r2, 10 of 10 files at r3, 2 of 2 files at r4, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @pav-kv)


pkg/kv/kvserver/raft_transport.go line 753 at r3 (raw file):

		// TODO(pav-kv): send these protos once they are populated correctly.
		if true {
			return

Any reason not to do this now, so it can be tested? I suppose TestFlowControlRaftTransportV2 did not test whether the outgoing message actually included the piggybacked state. Worth leaving a TODO in that test.


pkg/kv/kvserver/raft_transport.go line 770 at r3 (raw file):

		batch.Requests = batch.Requests[:0]
		batch.StoreIDs = nil
		batch.Now = hlc.ClockTimestamp{}

this needs to clear the AdmittedStates slice too.


pkg/kv/kvserver/raft_transport.go line 810 at r3 (raw file):

			var pendingDispatches []kvflowcontrolpb.AdmittedRaftLogEntries
			var admittedStates []kvflowcontrolpb.PiggybackedAdmittedState

nit: do we need this declared here, or can it be declared at usage time below.


pkg/kv/kvserver/raft_transport.go line 831 at r3 (raw file):

					kvadmission.FlowTokenDispatchMaxBytes.Get(&t.st.SV),
				)
				maybeAnnotateWithAdmittedRaftLogEntries(req, pendingDispatches)

if we want to reuse this test knob for v2, we should do the annotation here. We have the batch.


pkg/kv/kvserver/kvflowcontrol/node_rac2/admitted_piggybacker_test.go line 44 at r3 (raw file):

				d.ScanArgs(t, "store-id", &storeID)
				d.ScanArgs(t, "range-id", &rangeID)
				d.ScanArgs(t, "term", &term)

can you add the replicaID fields too.

@pav-kv pav-kv force-pushed the admission-vector-proto branch from 58cfecb to 60ecf7b Compare September 4, 2024 16:53
Copy link
Collaborator Author

@pav-kv pav-kv 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 @sumeerbhola)


pkg/kv/kvserver/raft_transport.go line 753 at r3 (raw file):

Previously, sumeerbhola wrote…

Any reason not to do this now, so it can be tested? I suppose TestFlowControlRaftTransportV2 did not test whether the outgoing message actually included the piggybacked state. Worth leaving a TODO in that test.

Added a TODO. Choosing not to send these messages just yet to be sure that none of this is on the wire in master/prod right now. Just in case we want to tweak these protos while developing (similarly to how we changed the proto definition in this PR in a backwards-incompatible way because we're sure these protos aren't populated yet). I want to enable the send when the replica integration is done and the leader can handle these updates, and both parts are covered by tests.


pkg/kv/kvserver/raft_transport.go line 770 at r3 (raw file):

Previously, sumeerbhola wrote…

this needs to clear the AdmittedStates slice too.

Done.


pkg/kv/kvserver/raft_transport.go line 810 at r3 (raw file):

Previously, sumeerbhola wrote…

nit: do we need this declared here, or can it be declared at usage time below.

Could be done, but after moving the annotation to the knob branch below (per other comment), the slice has to be defined in this line.


pkg/kv/kvserver/raft_transport.go line 831 at r3 (raw file):

Previously, sumeerbhola wrote…

if we want to reuse this test knob for v2, we should do the annotation here. We have the batch.

Done.


pkg/kv/kvserver/kvflowcontrol/node_rac2/admitted_piggybacker_test.go line 44 at r3 (raw file):

Previously, sumeerbhola wrote…

can you add the replicaID fields too.

Done.

Copy link
Collaborator

@sumeerbhola sumeerbhola 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 9 of 10 files at r6, 2 of 2 files at r7, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @pav-kv)

@pav-kv
Copy link
Collaborator Author

pav-kv commented Sep 4, 2024

TFTR!

bors r=sumeerbhola

@craig craig bot merged commit 6c51566 into cockroachdb:master Sep 4, 2024
23 checks passed
@pav-kv pav-kv deleted the admission-vector-proto branch September 4, 2024 20:51
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