Use correct cancelAfter broadcast when resources are exhausted #1767
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tylerkaraszewski
requested changes
Jun 4, 2024
sqlitecluster/SQLiteNode.cpp
Outdated
| @@ -1932,7 +1939,7 @@ void SQLiteNode::_changeState(SQLiteNodeState newState) { | |||
| // If we were following, and now we're not, we give up an any replications. | |||
| if (_state == SQLiteNodeState::FOLLOWING) { | |||
| _replicationThreadsShouldExit = true; | |||
| uint64_t cancelAfter = _leaderCommitNotifier.getValue(); | |||
| uint64_t cancelAfter = commitIDToCancelAfter > 0 ? commitIDToCancelAfter : _leaderCommitNotifier.getValue(); | |||
There was a problem hiding this comment.
I would just use the boolean value of commitIDToCancelAfter instead of comparing to 0.
sqlitecluster/SQLiteNode.cpp
Outdated
| // and waiting for the transaction that failed will be stuck in an infinite loop. To prevent that | ||
| // we're changing the state to SEARCHING and sending the cancelAfter property to drop all threads | ||
| // that depend on the transaction that failed to be threaded. | ||
| uint64_t cancelAfter = message.calcU64("NewCount") - 1; |
There was a problem hiding this comment.
Might want to check NewCount against 0 first, but this is an edge case that I don't think we can actually trigger.
tylerkaraszewski
approved these changes
Jun 4, 2024
johnmlee101
approved these changes
Jun 4, 2024
tylerkaraszewski
deleted the
dsilva_useCorrectCancelAfterWhenResourcesAreExhausted
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Details
Currently, we're facing issues with the replication timing. Because that's happening, the number of replication threads is growing too much, causing resource exhaustion.
Considering that all replication messages can arrive in parallel and not necessarily in order, followers could get stuck in the following case:
Since no threads were created for commit 2, it is never applied. Thread 2, which depends on commit 2 to be completed, gets stuck in an infinite loop. The server then never goes back to a searching state, and can only go back to the pool after a restart.
Fixed Issues
Fixes GH_LINK
Tests
Internal Testing Reminder: when changing bedrock, please compile auth against your new changes