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

Update Rufus-Scheduler version #11

Merged
merged 1 commit into from
May 26, 2017
Merged

Update Rufus-Scheduler version #11

merged 1 commit into from
May 26, 2017

Conversation

muz
Copy link
Contributor

@muz muz commented Aug 26, 2016

Uses a new (rewritten) version of Rufus-Scheduler.

This is mostly backwards compatible, and it works out of the box with our Dashboards; but others may find they require some minor compat changes to existing jobs. It's probably best to check with the rufus-scheduler README under the notable changes section if anything here is likely to affect your jobs.

I'm hoping that using a new Rufus-Scheduler won't exhibit some nasty memory leaking behaviour we observed where objects from previous jobs aren't properly dereferenced.

At the very least, this makes any future updates a bit less of a leap and a bit easier to accomodate for.

@contentfree
Copy link

How have the memory leaks been? Did they go away with the update to Rufus Scheduler 3?

@kinow
Copy link
Member

kinow commented Sep 22, 2016

Looked at Rufus-Scheduler 3, and at least from looking at app.rb, I found no problem in our simple usage creating a scheduler. Our default jobs in buzzwords, convergence, sample and twitter.rb should work fine too.

+1 for merging. Let's leave it open for a while, so that others can review this change too.

In the meantime, I'll try to test it locally, with a few sample jobs, and also try to write a couple of unit tests using a rufus scheduler and a few methods, so that next time we have to bump up the version we can check for simple backward compatibility (e.g. scheduler.every is a valid method).

@terraboops
Copy link

I'd like to merge this too, anyone have a chance to test it?

@dreezey
Copy link

dreezey commented Jan 6, 2017

Hi, I just forked the repo and changed the dependency, changed my Gemfile like so:
gem 'smashing', :git => 'https://github.com/dreezey/smashing.git'
Performed bundle update and restart my dashboard, everything worked without any code changes, however I'm using the "simple" version of the scheduler:
SCHEDULER.every '60s', :first_in => 0 do |job|

I'll keep an eye out the next couple of days for memory leaking (which was/is bothering my dashboard).

@MartynKeigher
Copy link

MartynKeigher commented Jan 6, 2017

@dreezey

One thing did with my scheduling was add a property to avoid overlapping.

SCHEDULER.every '60s', allow_overlapping: false, :first_in => 0 do |job|

Not saying its your "fix", but it may smooth a few things out for you.

@terraboops
Copy link

@MartynKeigher - The allow_overlapping: false flag is definitely best practice IMO. #37 will almost certainly set this as the default. Sensible defaults will be another benefit of having a job class! I want to make Smashing easy, so you don't have to spend a lot of time thinking and designing and can just get cool results quickly.

@dreezey
Copy link

dreezey commented Jan 11, 2017

@MartynKeigher the allow_overlapping property defintely helped! The dashboard is now running at a steady memory usage.

@adamstein-eb
Copy link

Any update on this? Any chance it could be merged?

@muz
Copy link
Contributor Author

muz commented Apr 10, 2017

I just pushed a rebased version of the branch and commit to avoid merge conflicts.

I haven't bumped the Rufus-Scheduler version beyond what it was before (3.2.0) although more recent versions have been released as I haven't actually tested backwards compat or drop in replace-ability with this newer version.

Glancing at the changelog, I note that they've dropped Ruby 1.9 support outright in the latest release; so those changes don't suggest to me that they're trivial.

For what it's worth, we've been running Dashing with this change on it for a while now and saw memory improvements and stability.

@terraboops terraboops merged commit 464aee1 into Smashing:master May 26, 2017
@muz muz deleted the rufus3 branch May 26, 2017 15:36
@kinow kinow added this to the 1.1.0 milestone May 19, 2020
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.

7 participants