Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Ability to remove workers after delay, conversion of workers to queue, callback when a worker is checked in and full pool status #106

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Ability to remove workers after delay, conversion of workers to queue, callback when a worker is checked in and full pool status #106

wants to merge 8 commits into from

Conversation

zugolosian
Copy link

@zugolosian zugolosian commented Jul 21, 2018 •
edited
Loading

#83 has been in use in a production environment for a while but there are some possible improvements for making it more efficient. I realise this pull request has a lot of changes but it would be great if we could figure out some of the common feature requests together so there aren't so many active forks.
This pull request:

  • Converts the workers to a queue for better lifo performance
  • Adds the ability to remove workers after a delay (reasoning detailed in Add ability to remove overflow workers after a delay #83)
  • Adds an optional callback so callers can perform any task they wish with the worker before checkin like a database connection reset, also gives the ability to kill/restart a worker on checkin if desired
  • Adds a full_status function which provides more information should someone want to graph pool changes
  • Adds a total workers started counter for detecting churn over time

David Leach and others added 7 commits July 21, 2018 19:34
Add ability to remove overflow workers after a delay
a0513eb 
When workers are expensive to start and transactions are quick killing
all workers that are checked in when in overflow is very expensive.
This change allows delaying the termination of overflow workers when
there is peak load and alleviates worker churn.
Update rebar3-eqc-plugin
466e7aa 
Closes devinus#77.
@walrusVision
Added function to return richer pool status info
0abd3ff
@GlenWalker
Don't reap workers that are checked out again
c486c14 
* Ensure reap message already in mailbox is flushed when cancelling reap
* Reap shouldn't be touching monitors, they are just for owner of checked out workers
Fix checkout stacktrace when noproc
25e76d3
Added support for OTP 21 stacktrace
447c568 
Converted available workers to a queue and single timer for overflow_ttl
@zugolosian
Merge branch 'master' into overflow_ttl_full_status_callback
45386b0
@zugolosian
Copy link
Author

I should also mention because of the use of monotonic time pre 17 is not supported

@zugolosian
Copy link
Author

Also happy to split this up a bit so each change can be looked at separately.

@zugolosian zugolosian force-pushed the overflow_ttl_full_status_callback branch from ffe58ef to 77caed3 Compare August 13, 2018 08:10
@Vagabond
Copy link
Collaborator

Yeah, we should really address this. I'm sorry we've taken so long on it.

I'm very busy and I'm not sure @devinus is around either, so anything you can do to make it easy to test would be appreciated.

@getong since you're also interested in helping maintain poolboy, can you pitch in here?

@getong
Copy link
Contributor

getong commented Aug 13, 2018

At first glance, it uses queue to handle workers, it is a very good idea.
Since some code was fixed in #104 , would you rebase your branch to master? @zugolosian

@zugolosian
Copy link
Author

Happy to rebase, will likely split the changes into:

  • Conversion to use a queue for workers
  • Full status information
  • Worker callback
  • Ability to remove workers after delay

That sound reasonable?

@Vagabond
Copy link
Collaborator

Thank you, yes. Also make sure you resolve the merge conflicts :)

@getong
Copy link
Contributor

getong commented Aug 17, 2018

rebar3_eqc has a new version 0.1.0, you can update to it.

@zugolosian
Copy link
Author

Sorry I haven't touched this in a while, been pretty busy myself. Shooting for sometime on Friday to separate into different pull requests.

@zugolosian zugolosian mentioned this pull request Sep 13, 2018
Merged
@zugolosian
Copy link
Author

@getong I've added a pull request for the conversion to a queue, let's work through that first and I'll start making the others so there aren't a bunch of conflicts to resolve

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@zugolosian @Vagabond @getong @walrusVision @GlenWalker