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

Fixed rescheduling jobs when mongodb timed out during execution of job. #112

Merged
merged 1 commit into from
Jan 31, 2017

Conversation

Kurounin
Copy link
Contributor

@Kurounin Kurounin commented Sep 9, 2016

When using external mongo db, if it timedout when running a scheduled job or when trying to update a job that has thrown an exception, the job would not be rescheduled to run again.
This fixes that.

@santiq
Copy link

santiq commented Sep 29, 2016

This is great!

@@ -225,11 +225,11 @@ SyncedCron._entryWrapper = function(entry) {
}
});
} catch(e) {
log.info('Exception "' + entry.name +'" ' + e.stack);
log.info('Exception "' + entry.name +'" ' + ((e && e.stack) ? e.stack : e));
Copy link

Choose a reason for hiding this comment

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

It will be better:
log.info('Exception "' + entry.name +'" ' + (e && e.stack || e));

@MichalW
Copy link

MichalW commented Dec 14, 2016

this issue is related to #69

@paolo-g
Copy link

paolo-g commented Jan 13, 2017

+1

@paolo-g paolo-g mentioned this pull request Jan 31, 2017
@paolo-g
Copy link

paolo-g commented Jan 31, 2017

@Kurounin were you able to apply @MichalW's suggestion? Any blocks to merging this other than admin approval?

@TheGame2500 TheGame2500 merged commit ee6014a into percolatestudio:master Jan 31, 2017
@Kurounin
Copy link
Contributor Author

Kurounin commented Feb 1, 2017

@paolo-g @MichalW's change produces the same result, it's just a shorter way of writing it. You can use any variant (e && e.stack) ? e.stack : e or e && e.stack || e. The way I wrote is more verbose.

@paolo-g
Copy link

paolo-g commented Feb 14, 2017

@Kurounin cool, I was trying to get the whole thing merged. @TheGame2500 do you know if there's a release planned that includes this patch? I have 1.3.0 in production at the moment, which I suppose doesn't include it.

@Kurounin
Copy link
Contributor Author

@paolo-g I've been using my fork since I made the PR. The PR was merged 15 days ago. @TheGame2500 will you release a new version on atmospherejs?

@TheGame2500
Copy link
Collaborator

@Kurounin unfortunately I can't do that, as I'm not authorised with my Galaxy account. @zol perhaps you could lend a hand with this?

@paolo-g
Copy link

paolo-g commented Mar 27, 2017

@zol bump <3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants