CNJR-2412: Set database connection pool size based on worker thread count #2875
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
micahlee
force-pushed
the
cnjr-2412-db-connection-pool
branch
3 times, most recently
from
31478c8 to
1e1e714
Compare
![codeclimate[bot]](https://avatars.githubusercontent.com/in/9403?s=60&v=4){kind=small}
| - New flag to `conjurctl server` command called `--no-migrate` which allows for skipping | ||
| the database migration step when starting the server. | ||
| [cyberark/conjur#2895](https://github.com/cyberark/conjur/pull/2895) | ||
|
|
||
| ### Changed | ||
| - The database thread pool max connection size is now based on the number of |
There was a problem hiding this comment.
Lists should be surrounded by blank lines
imheresamir
reviewed
Aug 15, 2023
imheresamir
reviewed
Aug 15, 2023
imheresamir
approved these changes
Aug 15, 2023
This fixes a "Fixed" block under the "Unreleased" section and a duplicate "Added" block in the version currently under development.
Rather than raise an ArgumentError, raise a message with details on which configuration value failed to parse and how to fix it.
Rather than configure puma directly in the appliance, use the Conjur application settings and config to set the puma threads.
micahlee
force-pushed
the
cnjr-2412-db-connection-pool
branch
from
1e1e714 to
58f8db5
Compare
|
Thanks @imheresamir, that was definitely a goof during history cleanup. That is fixed now. |
imheresamir
approved these changes
Aug 15, 2023
|
Code Climate has analyzed commit 58f8db5 and detected 1 issue on this pull request. Here's the issue category breakdown:
The test coverage on the diff in this pull request is 100.0% (50% is the threshold). This pull request will bring the total coverage in the repository to 88.4% (0.2% change). View more on Code Climate. |
|
@micahlee What was the thinking behind using 1.2 connections per thread? |
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.
Desired Outcome
The desired outcome of this PR is to prevent a database thread pool exhaustion in Conjur.
Implemented Changes
The primary change in this PR is to set the maximum database connections in the connection pool based on the number of threads per web worker, to mitigate the opportunity for connection exhaustion or for a request to time out waiting for a DB connection.
By default, the number of connections allowed in the pool is 1.2x the number of threads.
In addition to the core change this PR makes a couple of quality of life improvements that may be reviewed by commit:
Connected Issue/Story
CyberArk internal issue ID: CNJR-2412
Definition of Done
At least 1 todo must be completed in the sections below for the PR to be
merged.
Changelog
CHANGELOG update
Test coverage
changes, or
We do not currently have any test suites that can execute tests at the scale to exhibit the faulty behavior this PR addresses. This approach, however, was validated in the field, and the overall configuration approach is validated through our E2E test suites.
Documentation
READMEs) were updated in this PRBehavior
Security