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

rac2: protect from unlimited force-flush #135814

Closed
pav-kv opened this issue Nov 20, 2024 · 0 comments
Closed

rac2: protect from unlimited force-flush #135814

pav-kv opened this issue Nov 20, 2024 · 0 comments
Assignees
@sumeerbhola
Labels
A-replication-admission-control-v2 Related to introduction of replication AC v2 C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-kv KV Team

Comments

@pav-kv
Copy link
Collaborator

pav-kv commented Nov 20, 2024 •
edited by exalate-issue-sync bot
Loading

The force-flush mechanism bypasses token waiting and optimistically/eagerly replicates the log to a peer. There is no pacing/limiting. If the peer doesn't send any MsgAppResps for a bit, we can accumulate a large in-flight window. If many ranges do that simultaneously, this can lead to OOM.

Previous raft behaviour is eager too, but it has max-inflight limits per leader->peer flow and works mostly well, rarely having issues.

We should consider adopting this previous raft behaviour in RACv2 for force-flushing.

Jira issue: CRDB-44733

Epic CRDB-42900
@pav-kv pav-kv added C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) A-replication-admission-control-v2 Related to introduction of replication AC v2 T-kv KV Team labels Nov 20, 2024
sumeerbhola added a commit to sumeerbhola/cockroach that referenced this issue Dec 7, 2024
@sumeerbhola
kvserver,rac2: use RaftMaxInflightBytes when force-flushing
c1cf6fb 
This limit is roughly respected, in that force-flush is paused when the
limit is exceeded, and unpaused when ready handling indicates that the
replicaSendStream is back within the limit.

Informs cockroachdb#135814

Epic: none

Release note: None
@sumeerbhola sumeerbhola mentioned this issue Dec 7, 2024
Merged
sumeerbhola added a commit to sumeerbhola/cockroach that referenced this issue Dec 10, 2024
@sumeerbhola
rac2: improve TestRangeController wrt match and inflight-bytes
c800959 
This is in preparation for RangeController needing the per-replica
inflight bytes. The existing test was unnecessarily and implicitly
changing match. This happened in SendMsgAppRaftMuLocked, handling
of set_replicas and raft_event. This is now changed to only adjust
match explicitly via admit. And set_replicas will set match to 0,
but there is now a provision to explicitly provide a match value,
which is used to preserve an existing match value.

This permits maintaining ReplicaStateInfo.InflightBytes (which is
currently only relevant to the test) in the test code, by adjusting
it when match and next are changed. This is done via looking at
entries in MemoryStorage. The wait_for_eval_send_q is slightly modified,
since it needs to actually send the entry at index 1, so that it
can be retrieved from MemoryStorage.

Informs cockroachdb#135814

Epic: CRDB-37515

Release note: None
@sumeerbhola sumeerbhola mentioned this issue Dec 10, 2024
Merged
sumeerbhola added a commit to sumeerbhola/cockroach that referenced this issue Dec 10, 2024
@sumeerbhola
rac2: improve TestRangeController wrt match and inflight-bytes
4fdc203 
This is in preparation for RangeController needing the per-replica
inflight bytes. The existing test was unnecessarily and implicitly
changing match. This happened in SendMsgAppRaftMuLocked, handling
of set_replicas and raft_event. This is now changed to only adjust
match explicitly via admit. And set_replicas will set match to 0,
but there is now a provision to explicitly provide a match value,
which is used to preserve an existing match value.

This permits maintaining ReplicaStateInfo.InflightBytes (which is
currently only relevant to the test) in the test code, by adjusting
it when match and next are changed. This is done via looking at
entries in MemoryStorage. The wait_for_eval_send_q is slightly modified,
since it needs to actually send the entry at index 1, so that it
can be retrieved from MemoryStorage.

Informs cockroachdb#135814

Epic: CRDB-37515

Release note: None
sumeerbhola added a commit to sumeerbhola/cockroach that referenced this issue Dec 10, 2024
@sumeerbhola
kvserver,rac2: use RaftMaxInflightBytes when force-flushing
96f23b6 
This limit is roughly respected, in that force-flush is paused when the
limit is exceeded, and unpaused when ready handling indicates that the
replicaSendStream is back within the limit.

Informs cockroachdb#135814

Epic: none

Release note: None
sumeerbhola added a commit to sumeerbhola/cockroach that referenced this issue Dec 10, 2024
@sumeerbhola
kvserver,rac2: use RaftMaxInflightBytes when force-flushing
bcd5030 
This limit is roughly respected, in that force-flush is paused when the
limit is exceeded, and unpaused when ready handling indicates that the
replicaSendStream is back within the limit.

Informs cockroachdb#135814

Epic: none

Release note: None
craig bot pushed a commit that referenced this issue Dec 10, 2024
@aadityasondhi @rharding6373 @sumeerbhola
Merge #136782 #137032 #137070
9f28bc0 
136782: roachtest: respect ctx cancelation in admission-control/elastic-io r=srosenberg,DarrylWong,herkolategan a=aadityasondhi

Fixes #136557.

Release note: None

137032: changefeedccl: skip some sql errors in random expr changefeed test r=asg0451 a=rharding6373

There are some SQL errors that are encountered by CDC queries that aren't encountered by a SQL query using the same filter. These happen in queries that have both tautologies and invalid SQL once the values in the row are processed. In a SQL query, these queries return the result of the row without processing the remainder of the row, thus not encountering the. In a CDC query, the entire query is processed, so if there is invalid SQL it is caught here.

This change modifies TestChangefeedRandomExpressions to skip tests where certain invalid SQL is found after the CDC query is executed but passed the SQL query stage of the test.

Fixes: #135269
Fixes: #134813
Fixes: #133049
Fixes: #127642
Fixes: #124738
Fixes: #120174
Fixes: #137038

Release note: None

137070: rac2: improve TestRangeController wrt match and inflight-bytes r=kvoli a=sumeerbhola

This is in preparation for RangeController needing the per-replica inflight bytes. The existing test was unnecessarily and implicitly changing match. This happened in SendMsgAppRaftMuLocked, handling of set_replicas and raft_event. This is now changed to only adjust match explicitly via admit. And set_replicas will set match to 0, but there is now a provision to explicitly provide a match value, which is used to preserve an existing match value.

This permits maintaining ReplicaStateInfo.InflightBytes (which is currently only relevant to the test) in the test code, by adjusting it when match and next are changed. This is done via looking at entries in MemoryStorage. The wait_for_eval_send_q is slightly modified, since it needs to actually send the entry at index 1, so that it can be retrieved from MemoryStorage.

Informs #135814

Epic: CRDB-37515

Release note: None

Co-authored-by: Aaditya Sondhi <[email protected]>
Co-authored-by: rharding6373 <[email protected]>
Co-authored-by: sumeerbhola <[email protected]>
sumeerbhola added a commit to sumeerbhola/cockroach that referenced this issue Dec 10, 2024
@sumeerbhola
kvserver,rac2: use RaftMaxInflightBytes when force-flushing
6391681 
This limit is roughly respected, in that force-flush is paused when the
limit is exceeded, and unpaused when ready handling indicates that the
replicaSendStream is back within the limit.

Informs cockroachdb#135814

Epic: none

Release note: None
craig bot pushed a commit that referenced this issue Dec 11, 2024
@mgartner @sumeerbhola @arulajmani
Merge #136861 #136977 #137313
53fadf4 
136861: sql: separate pausable portal code paths in `execStmtInOpenState` r=mgartner a=mgartner

#### sql: add `(*connExecutor).execStmtInOpenStateWithPausablePortal`

The `execStmtInOpenStateWithPausablePortal` method has been added to
`connExecutor`. It is currently an exact copy of `execStmtInOpenState`.
Future commits will simplify both methods.

Release note: None

#### sql: remove `isPausablePortal` anonymous function in `execStmtInOpenStateWithPortal`

Release note: None

#### sql: remove pausable-portal-specific code paths in `execStmtInOpenState`

Release note: None

#### sql: remove `processCleanupFunc` in `execStmtInOpenState`

Fixes #135908

Release note: None

#### sql: remove `portal` parameter from `execStmtInOpenState`

Release note: None

#### sql: remove `localVars` struct in `execStmtInOpenState`

Refactoring in previous commits has prevented all the `localVars`
variables from escaping to the heap, except for `cancelQuery`. So the
struct is no longer needed. Removing it reduces the size of heap
allocations, since the entire struct was heap allocated previously.

Release note: None

#### sql: refactor errors in `execStmtInOpenState`

Release note: None

#### sql: add metamorphic test variable to force pausable portal logic

Release note: None

136977: kvserver,rac2,tracker: use RaftMaxInflightBytes when force-flushing r=sumeerbhola a=sumeerbhola

This limit is roughly respected, in that force-flush is paused when the
limit is exceeded, and unpaused when ready handling indicates that the
replicaSendStream is back within the limit.

Informs #135814

Epic: none

Release note: None

137313: raft: misc becomeFollower cleanups r=nvanbenschoten a=arulajmani

See individual commits. 

Co-authored-by: Marcus Gartner <[email protected]>
Co-authored-by: sumeerbhola <[email protected]>
Co-authored-by: Arul Ajmani <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@sumeerbhola sumeerbhola

Labels
A-replication-admission-control-v2 Related to introduction of replication AC v2 C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-kv KV Team
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@pav-kv @sumeerbhola