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.

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

dont try and unlock jobs when _lockedJobs is empty #509

Merged
merged 3 commits into from Aug 16, 2017
Merged

Conversation

@OmgImAlexis
Copy link
Member

OmgImAlexis commented Aug 16, 2017

This removes a useless database call since the find statement will return nothing if an empty array is passed.

@OmgImAlexis OmgImAlexis requested a review from simison Aug 16, 2017
debug('about to unlock jobs with ids: %O', jobIds);
self._collection.updateMany({_id: {$in: jobIds}}, {$set: {lockedAt: null}}, done);

if (jobIds.length >= 1) {

This comment has been minimized.

Copy link
@simison

simison Aug 16, 2017

Member

BTW for readability might be worth considering exiting early if you don't mind multiple return points:

if (!jobIds.length) {
  debug('no jobs to unlock');
  return done();
}

// ...
return done();

But I'm quite indifferent regarding these.

This comment has been minimized.

Copy link
@OmgImAlexis

OmgImAlexis Aug 16, 2017

Author Member

Length should always be compared. If length is missing it'll return undefined. I'll remove the unneeded return though, added this after being up 48+ hours. Oops.

> !undefined
true

This comment has been minimized.

Copy link
@simison

simison Aug 16, 2017

Member

I meant that instead of but I'm just bikeshedding really. :-)

if jobs
 // unlock jobs
else
  // no jobs

could be

if !jobs
 // return no jobs

// unlock jobs

This comment has been minimized.

Copy link
@OmgImAlexis

OmgImAlexis Aug 16, 2017

Author Member

Ah. Should be checking for errors before running application code. Switching.

return done(err);
}

self._lockedJobs = [];

This comment has been minimized.

Copy link
@simison

simison Aug 16, 2017

Member

Is there a test case for this change? (If it even is a change, but looks like previously it would just leave this array behind).

This comment has been minimized.

Copy link
@OmgImAlexis

OmgImAlexis Aug 16, 2017

Author Member

Before it'd just leave it so I just set it back to how it was when we instantiate Agenda which is [].

I'll add a test to the add-ava branch.

Copy link
Member

simison left a comment

LGTM!

Great to see optimisations!

OmgImAlexis added 2 commits Aug 16, 2017
simison added a commit that referenced this pull request Aug 16, 2017
@OmgImAlexis OmgImAlexis merged commit 9be8ab8 into master Aug 16, 2017
1 of 3 checks passed
1 of 3 checks passed
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details
codeclimate All good!
Details
@OmgImAlexis OmgImAlexis deleted the _unlockJobs branch Aug 16, 2017
@OmgImAlexis OmgImAlexis added this to the 1.1.0 milestone Aug 16, 2017
timelf123 added a commit to ideawake/agenda that referenced this pull request Feb 16, 2018
timelf123 added a commit to ideawake/agenda that referenced this pull request Feb 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.