Skip to content
This repository has been archived by the owner on Jul 25, 2022. It is now read-only.

Make db pool size configurable #2158

Merged
merged 4 commits into from
Aug 17, 2020
Merged

Make db pool size configurable #2158

merged 4 commits into from
Aug 17, 2020

Conversation

utopiabound
Copy link
Contributor

@utopiabound utopiabound commented Aug 13, 2020 •
edited by jgrund
Loading

Allow override of POOL_LIMIT on a per service basis
Adjust most limits down
Adjust task-runner limit up to 10

Fixes #2157

Signed-off-by: Nathaniel Clark nclark@whamcloud.com

This change is Reviewable

@utopiabound
Make db pool size configurable
9b8a286 
Allow override of POOL_LIMIT on a per service basis
Adjust most limits down
Adjust task-runner limit up to 10

Fixes #2157

Signed-off-by: Nathaniel Clark <nclark@whamcloud.com>
@utopiabound utopiabound self-assigned this Aug 13, 2020
jgrund
jgrund reviewed Aug 13, 2020
View reviewed changes
iml-mailbox/src/main.rs Outdated Show resolved Hide resolved
jgrund
jgrund reviewed Aug 13, 2020
View reviewed changes
iml-api/src/main.rs Outdated Show resolved Hide resolved
jgrund
jgrund reviewed Aug 13, 2020
View reviewed changes
iml-task-runner/src/main.rs Outdated Show resolved Hide resolved
jgrund
jgrund reviewed Aug 13, 2020
View reviewed changes
Copy link
Member

@jgrund jgrund left a comment

Choose a reason for hiding this comment

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

Small nit on comment spelling, otherwise LGTM.

@jgrund jgrund self-requested a review August 13, 2020 15:16
jgrund
jgrund suggested changes Aug 13, 2020
View reviewed changes
Copy link
Member

@jgrund jgrund left a comment

Choose a reason for hiding this comment

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

Looks like rust check is failing

@utopiabound utopiabound marked this pull request as draft August 13, 2020 15:21
@utopiabound
Fix spelling and add LIMIT to task-runner
aea9cff 
Signed-off-by: Nathaniel Clark <nclark@whamcloud.com>
@utopiabound utopiabound marked this pull request as ready for review August 13, 2020 17:34
@jgrund
update sqlx-data.json
e426ace 
Signed-off-by: Joe Grund <jgrund@whamcloud.io>
ip1981
ip1981 previously approved these changes Aug 14, 2020
View reviewed changes
jgrund
jgrund suggested changes Aug 14, 2020
View reviewed changes
Copy link
Member

@jgrund jgrund left a comment

Choose a reason for hiding this comment

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

Seeing this during testing:

iml_ntp.1.i9q1wco2pxzr@ai400-0a9c-vm00    | thread 'main' panicked at 'POOL_LIMIT environment variable is required.', iml-manager-env/src/lib.rs:28:39
iml_ntp.1.bjst8ft5dg8q@ai400-0a9c-vm00    | Starting dependency check

In get_pool_limit, use env::var instead of get_var. get_var will panic if it can't find the variable

@utopiabound
Address review comments
d1e72e2 
Signed-off-by: Nathaniel Clark <nclark@whamcloud.com>
ip1981
ip1981 reviewed Aug 14, 2020
View reviewed changes
Copy link
Member

@ip1981 ip1981 left a comment

Choose a reason for hiding this comment

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

Now it's inconsistently pool_limit or default_pool_limit in various places.

My point about the naming was mostly avoiding comments by using appropriate names :)

ip1981
ip1981 reviewed Aug 14, 2020
View reviewed changes
iml-mailbox/src/main.rs
use std::pin::Pin;
use warp::Filter as _;

// Default pool limit if not overridden by POOL_LIMIT
lazy_static! {
Copy link
Member

Choose a reason for hiding this comment

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

Why lazy_static?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So it's only eval'd once, and this allows it to be used inside the async move without passing in as an extra filter.

@jgrund jgrund requested a review from ip1981 August 15, 2020 00:24
@jgrund
Copy link
Member

jgrund commented Aug 15, 2020

I've tested this on google cloud with 65 mounted clients and 66 servers. All fids were processed and active_clients were empty afterwards

-[ RECORD 1 ]---+-----------------------------------------------------------------------------------
id              | 7
name            | 38303150-094b-4dd1-abf0-5fca2948b0a7-warn_fids-fids_expiring_soon
start           | 2020-08-15 00:30:22.519986+00
finish          | 2020-08-15 00:31:02.172049+00
state           | removed
fids_total      | 216301
fids_completed  | 216301
fids_failed     | 0
data_transfered | 0
single_runner   | f
keep_failed     | f
actions         | {stratagem.warning}
args            | {"report_name": "expiring_fids-exacloud-38303150-094b-4dd1-abf0-5fca2948b0a7.txt"}
filesystem_id   | 1
running_on_id   |

@jgrund jgrund merged commit ca5f9e1 into master Aug 17, 2020
@jgrund jgrund deleted the pool-limit branch August 17, 2020 13:11
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@johnsonw johnsonw johnsonw approved these changes

@jgrund jgrund jgrund approved these changes

@ip1981 ip1981 Awaiting requested review from ip1981

Assignees

@utopiabound utopiabound

Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

iml-task-runner: limit parallelism to number of connections in pool
4 participants
@utopiabound @jgrund @ip1981 @johnsonw