Skip to content

Conversation

@timburks
Copy link
Contributor

@timburks timburks commented Sep 3, 2022

This applies fairly tight restrictions to database connection usage. When all connections are in use, the server blocks until one becomes available. This can cause operations to take longer when concurrency is high, but by not consuming all available connections, this keeps the database available for other clients. As noted in the comments, I think we may want to make this configurable.

@codecov
Copy link

codecov bot commented Sep 3, 2022

Codecov Report

Merging #725 (8a430e0) into main (8e82f10) will increase coverage by 0.07%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main     #725      +/-   ##
==========================================
+ Coverage   57.13%   57.21%   +0.07%     
==========================================
  Files          93       93              
  Lines        8005     8005              
==========================================
+ Hits         4574     4580       +6     
+ Misses       3003     2999       -4     
+ Partials      428      426       -2     
Impacted Files Coverage Δ
server/registry/actions_specs.go 79.72% <0.00%> (+1.35%) ⬆️
server/registry/actions_deployments.go 77.84% <0.00%> (+1.79%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@timburks timburks requested a review from theganyo September 6, 2022 14:50
}
if err := applyConnectionLimits(db); err != nil {
c := &Client{db: db}
c.close()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, I find this to be a strange pattern we follow: Create a Client in order to call close() on it, which simply calls close() on the db we just constructed it on, only to then throw the client away.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good topic -- it looks like it's allowing us to keep the following code in one place:

	sqlDB, _ := c.db.DB()
	sqlDB.Close()

That could also be done with Client{db:db}.close()... or directly using the above code.

I don't have a strong preference. @seaneganx ?

@timburks timburks merged commit 86315f3 into apigee:main Sep 6, 2022
@timburks timburks deleted the connection-limits branch October 19, 2022 16:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

2 participants