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

fix(pool): added optional setting for killing processes and not retaining them #1173

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from
Open

fix(pool): added optional setting for killing processes and not retaining them #1173

wants to merge 5 commits into from

Conversation

dawiss1337
Copy link

Created optional setting for child pool, default is set to true - always retains child processes, but when set to false the child process is killed and not retained after finishing processing.

@dawiss1337
Copy link
Author

@manast would appreciate if you could take a look at this PR

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.06%) to 93.789% when pulling ac446e4 on DavisJaunzems:master into 40a69ac on OptimalBits:develop.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.06%) to 93.789% when pulling ac446e4 on DavisJaunzems:master into 40a69ac on OptimalBits:develop.

@coveralls
Copy link

coveralls commented Jan 14, 2019 •
edited

Coverage Status

Coverage decreased (-0.01%) to 93.836% when pulling 2b5bfdc on DavisJaunzems:master into 40a69ac on OptimalBits:develop.

@manast
Copy link
Member

manast commented Jan 17, 2019

@DavisJaunzems sorry for the late response. It has been a hectic week so far.

manast
manast reviewed Jan 17, 2019
View reviewed changes
lib/job.js Outdated
@@ -48,7 +48,7 @@ function setDefaultOpts(opts) {
var _opts = Object.assign({}, opts);

_opts.attempts = typeof _opts.attempts == 'undefined' ? 1 : _opts.attempts;
_opts.delay = typeof _opts.delay == 'undefined' ? 0 : _opts.delay;
_opts.delay = typeof _opts.delay == 'undefined' ? 0 : Number(_opts.delay);
Copy link
Member

Choose a reason for hiding this comment

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

this fix seems to have been slipped in from an unrelated issue.

test/test_child-pool.js Outdated

return pool.retain(processor).then(function(_child) {
expect(_child).to.be.ok;
child = _child;
Copy link
Member

Choose a reason for hiding this comment

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

Why this assignment?

Copy link
Author

Choose a reason for hiding this comment

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

Must have been from copy-paste, you are right no need for re-assigned the variable, could pool.release(_child); directly

Copy link
Author

Choose a reason for hiding this comment

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

Updated PR, removed the unnecessary reassignment

manast
manast requested changes Jan 20, 2019
View reviewed changes
lib/process/child-pool.js Outdated
this.retained = {};
this.free = {};
};

ChildPool.prototype.setKeepProcesses = function(keepProcesses) {
Copy link
Member

Choose a reason for hiding this comment

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

I am thinking that it would be better to change the name to a more explanatory: setReuseProcesses or similar.

Copy link
Author

Choose a reason for hiding this comment

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

renamed the methods and variables in the latest PR

lib/queue.js Outdated
@@ -250,6 +251,13 @@ var Queue = function Queue(name, url, opts) {
this.processJob = this.processJob.bind(this);
this.getJobFromId = Job.fromId.bind(null, this);

// Check settings to see if processes will be kept after finishing processing, default set to true
this.keepProcesses =
Copy link
Member

Choose a reason for hiding this comment

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

Since we are setting a default value true a few lines above, this is not necessary.

lib/queue.js Outdated
@@ -672,6 +680,10 @@ Queue.prototype.start = function(concurrency) {
});
};

Queue.prototype.setKeepProcesses = function(keepProcesses) {
Copy link
Member

Choose a reason for hiding this comment

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

I do not think we need a setter for this setting, as with the other settings.

@dawiss1337
Copy link
Author

The latest commit has changed method and variable names, and removed redundant code @manast

@manast
Copy link
Member

manast commented Jan 21, 2019

There are 3 format issues preventing the tests from running:

/home/travis/build/OptimalBits/bull/test/test_child-pool.js
  145:74  error  Unexpected function expression            prefer-arrow-callback
  146:5   error  Unexpected var, use let or const instead  no-var
  150:40  error  Unexpected function expression            prefer-arrow-callback

@dawiss1337
Copy link
Author

@manast what command did you run to get these errors, because I can run all of the tests locally?
Also looking at the format issues (using let or const instead of var and using arrow functions), all of the tests should have the same formatting issues..

@manast
Copy link
Member

manast commented Jan 30, 2019

@DavisJaunzems The command that you can run locally:
npm run prettier -- --list-different

@dawiss1337
Copy link
Author

@manast updated the code, anything else before this can be merged?

@ks-s-a
Copy link

ks-s-a commented Aug 20, 2019

The PR was spoiled, but the feature is good. @manast may it be merged for the next release?

@manast
Copy link
Member

manast commented Aug 20, 2019

In order to merge this we need to rework it according to the comments and make it pass the tests.

@stale
Copy link

stale bot commented Jul 12, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label Jul 12, 2021
@stale stale bot removed the wontfix label Jul 15, 2021
@manast manast added the pinned label Jul 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@manast manast manast requested changes

Assignees
No one assigned
Labels
pinned
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@dawiss1337 @coveralls @manast @ks-s-a