chore: Merge session connection deadlock branch to main #1919
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
github-actions
bot
added
core
core/db
core/servers
core/session
core/sql
docs/concepts
website
labels
…ry on grace period expiration
The query for CloseDeadConnectionsForWorker includes a time comparison to exclude results that are too close to the current time. This is done to account for a delay in reporting by the worker. However, since the delay was hardcoded in the query, it made it difficult to test. This moves the delay duration into a parameter that is set on the repository. The amount of the delay can be set at repository creation time using the new WithWorkerStateDelay option. This defaults to the previously hardcoded amount. This will allow tests to control the delay to exercise this functionality fully.
This adds tests for the Status method of the WorkerService grpc service that is exposed by controller.
irenarindos
force-pushed
the
llb-session-connection-deadlocks
branch
from
f5ec798 to
90a5367
Compare
This creates a domain service function `session.WorkerStatusReport`. The service performs two functions: 1. Checks the status for all of the reported sessions, returning a report of the sessions that are in a "non-active" state, i.e. "cacneling" or "terminated". 2. Looks for any "orphaned" session connections, i.e. connections that are active but were not reported by the worker. These connections are marked as closed. This also removes the "ShouldCloseConnectionsOnWorker" logic, as this was determined to not be a possible state, and was unnecessary.
This removes or reduces the time that is waited in a number of tests by allowing the grace period time be a configuration option on the connection repository.
tmessi
force-pushed
the
llb-session-connection-deadlocks
branch
from
90a5367 to
bbc12d4
Compare
tmessi
approved these changes
Mar 15, 2022
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.
The deadlock reported in #1812 was caused by contention between processes for session connection closure. The same connection was closed in more than one place, and connections were closed across several sessions. For multiple connection closures, multiple sessions could be updated in the same transaction. Additionally, database triggers for data warehousing would update session connection and session warehouse tables in the same transaction.
The changes in this pull request cover moving session connections into a separate repository from sessions. Transactions for connections and sessions are handled separately to eliminate the resource contention causing a deadlock. Domain services have also been added to session package to implement functionality that requires accessing both the session and session connection repositories.
Fixes #1812