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

Improve load speed from the database by 1000% (and fix specs and add Rails 4 support) #521

Closed
wants to merge 33 commits into
base: master
from

Conversation

Projects
None yet
7 participants
@cheald
Member

cheald commented Jun 22, 2013

The tl;dr here is that MongoMapper is doing some really ugly things - notably, letting the Dirty plugin get its hands on content as it's being loaded from the DB, and performing #extends on instance variables, which is torching the entire Ruby method cache on a regular basis. This PR fixes this.

First, let's benchmark!

I was interested in comparing Mongoid with MongoMapper, and was rather dismayed to find how abysmally MM was performing. It's worth noting that Mongoid uses the Moped driver, while MM uses the 10gen Mongo driver (which, IMO, is something of a mess; part of the blame is due there).

The benchmark is here: https://github.com/cheald/mongo_odm_bench

This comparison is MongoMapper 0.12.0 vs Mongoid 3.1.4

       user     system      total        real
MongoMapper Insert  0.480000   0.040000   0.520000 (  0.542837)
    Mongoid Insert  0.530000   0.040000   0.570000 (  0.574484)

       user     system      total        real
MongoMapper Read 150        batches of 1           0.170000   0.010000   0.180000 (  0.212139)
    Mongoid Read 150        batches of 1           0.050000   0.010000   0.060000 (  0.066210)
MongoMapper Read 3750       batches of 25          1.700000   0.180000   1.880000 (  1.937555)
    Mongoid Read 3750       batches of 25          0.180000   0.020000   0.200000 (  0.211497)
MongoMapper Read 15000      batches of 100         6.540000   0.620000   7.160000 (  7.263680)
    Mongoid Read 15000      batches of 100         0.620000   0.050000   0.670000 (  0.704031)
MongoMapper Read 75000      batches of 500        32.200000   2.960000  35.160000 ( 35.474875)
    Mongoid Read 75000      batches of 500         2.930000   0.280000   3.210000 (  3.277081)
MongoMapper Read 150000     batches of 1000       64.250000   5.950000  70.200000 ( 70.786035)
    Mongoid Read 150000     batches of 1000        5.810000   0.580000   6.390000 (  6.499160)

Now, with my patches:

       user     system      total        real
MongoMapper Insert  0.740000   0.080000   0.820000 (  1.205486)
    Mongoid Insert  0.530000   0.040000   0.570000 (  0.580322)
       user     system      total        real
MongoMapper Read 150        batches of 1           0.050000   0.000000   0.050000 (  0.068624)
    Mongoid Read 150        batches of 1           0.060000   0.010000   0.070000 (  0.070675)
MongoMapper Read 3750       batches of 25          0.190000   0.030000   0.220000 (  0.234519)
    Mongoid Read 3750       batches of 25          0.180000   0.020000   0.200000 (  0.215139)
MongoMapper Read 15000      batches of 100         0.600000   0.110000   0.710000 (  0.745613)
    Mongoid Read 15000      batches of 100         0.600000   0.060000   0.660000 (  0.692381)
MongoMapper Read 75000      batches of 500         2.960000   0.440000   3.400000 (  3.471220)
    Mongoid Read 75000      batches of 500         2.890000   0.280000   3.170000 (  3.227801)
MongoMapper Read 150000     batches of 1000        5.830000   0.950000   6.780000 (  6.896435)
    Mongoid Read 150000     batches of 1000        5.710000   0.550000   6.260000 (  6.375876)

The time to read 15k records was reduced from 70.7 seconds to 6.89 seconds, an improvement of 1026%. I'm pretty sure that the difference in insert time is due to the updated "safe" parameter, which I didn't explicitly set in the 0.12.0 config.

[Edit: And here's for the current repo head, which is much better, but I've managed to ~triple performance even over that!)

       user     system      total        real
MongoMapper Insert  0.720000   0.080000   0.800000 (  1.222013)
    Mongoid Insert  0.520000   0.040000   0.560000 (  0.557910)
       user     system      total        real
MongoMapper Read 150        batches of 1           0.080000   0.010000   0.090000 (  0.100922)
    Mongoid Read 150        batches of 1           0.050000   0.000000   0.050000 (  0.065750)
MongoMapper Read 3750       batches of 25          0.580000   0.080000   0.660000 (  0.679814)
    Mongoid Read 3750       batches of 25          0.180000   0.010000   0.190000 (  0.221572)
MongoMapper Read 15000      batches of 100         2.100000   0.240000   2.340000 (  2.381593)
    Mongoid Read 15000      batches of 100         0.620000   0.080000   0.700000 (  0.718032)
MongoMapper Read 75000      batches of 500        10.160000   1.140000  11.300000 ( 11.426766)
    Mongoid Read 75000      batches of 500         2.920000   0.320000   3.240000 (  3.302835)
MongoMapper Read 150000     batches of 1000       20.120000   2.350000  22.470000 ( 22.671705)
    Mongoid Read 150000     batches of 1000        5.860000   0.550000   6.410000 (  6.525847)
  • The big change here is that when loading from the database, we use an #internal_write_key method that isn't eligible for plugins to overwrite. Previously, things like the Dirty plugin's #write_key would be invoked when loading from the DB, which in turn invoked read_key and a whole other chain of things. This massively slowed down read
    speeds.
  • In two places, runtime instances were being #extend'd. This wipes the Ruby method cache and slows down the entire application. Those two instances have been refactored out.
  • Other changes have been made to improve #internal_write_key. First off, uncast values are now stored in write (by the app, not the DB), and only then from the Rails plugin, as that's the functionality that is concerned with uncast values. It's now a simple hash lookup rather than being a whole mess of ivars.
  • Keys#keys now copies the class.keys to an ivar when it's used. This method is used very frequently, and avoiding the extra function call has substantial benefit when you start adding it up.
  • is_a? was replaced with instance_of? in a number of places; this is faster when you want to compare types directly, rather than the inheritance hierarchiy.
  • We're testing !foo rather than foo.nil? in a number of places for similar reasons.
  • In a few places, ivars were used rather than their prettier accessors. Avoiding the function call does add up when you're calling it hundreds of thousands of times.
  • Several tests which were too brittle have been fixed.

I'm quite certain that there are a number of other places to make improvements, but I'm pretty happy with a 10x improvement for an evening's worth of hacking.

cheald added some commits Jun 18, 2013

Remove instance #extends. They torch the ruby method cache! This is V…
…ery Very Bad. Don't do identity map stuff when the identity map is disabled.
Big changes to load behavior to improve load speeds substantially.
The big change here is that when loading from the database, we use
an #internal_write_key method that isn't eligible for plugins to
overwrite. Previously, things like #dirty_key's write_key would
be invoked when loading from the DB, which in turn invoked read_key
and a whole other chain of things. This massively slowed down read
speeds.

Other changes have been made to improve internal_write_key. First off,
uncast values are now stored in write (by the app, not the DB), and
only then from the Rails plugin, as that's the functionality that is
concerned with uncast values. It's now a simple hash lookup rather
than being a whole mess of ivars.

Keys#keys now copies the class.keys to an ivar when it's used. This
method is used very frequently, and avoiding the extra function
call has substantial benefit when you start adding it up.

is_a? was replaced with instance_of? in a number of places; this is
faster when you want to compare types directly, rather than the
inheritance hierarchiy.

We're testing !foo rather than foo.nil? in a number of places for
similar reasons.

Several tests which were too brittle have been fixed.

These changes have resulted in something approaching a 1000% speed
increase when reading from the database.
Show outdated Hide outdated lib/mongo_mapper/extensions/time.rb
@@ -3,7 +3,7 @@ module MongoMapper
module Extensions
module Time
def to_mongo(value)
if !value.present?
if !value || value === ''

This comment has been minimized.

@zanker

zanker Jun 22, 2013

This causes errors under JRuby 1.7.2 through 1.7.4 (at least), doing Time.now === "" triggers a no implicit conversion from nil to integer. Arguably this is a JRuby issue as that works fine under MRI, but it makes it impossible to set any time values on JRuby through MM.

Perhaps change this to the check used for Extensions::Date?

if !value || (value.instance_of?(String) && '' === value)

@zanker

zanker Jun 22, 2013

This causes errors under JRuby 1.7.2 through 1.7.4 (at least), doing Time.now === "" triggers a no implicit conversion from nil to integer. Arguably this is a JRuby issue as that works fine under MRI, but it makes it impossible to set any time values on JRuby through MM.

Perhaps change this to the check used for Extensions::Date?

if !value || (value.instance_of?(String) && '' === value)

This comment has been minimized.

@cheald

cheald Jun 22, 2013

Member

What is value in that case? I can't produce the error in a 1.7.1 console. Trying 1.7.4 now.

@cheald

cheald Jun 22, 2013

Member

What is value in that case? I can't produce the error in a 1.7.1 console. Trying 1.7.4 now.

This comment has been minimized.

@zanker

zanker Jun 22, 2013

value is any sort of time. Reinstalled JRuby 1.7.1 and it happens on that too.

@zanker

zanker Jun 22, 2013

value is any sort of time. Reinstalled JRuby 1.7.1 and it happens on that too.

cheald added some commits Jun 22, 2013

Add some comments to the rather arcane @Keys assignments. I don't lik…
…e them code-wise but they're definitely faster than a method call. Maybe too optimize-y?
@arthurnn

This comment has been minimized.

Show comment
Hide comment
@arthurnn

arthurnn Jun 22, 2013

Contributor

@cheald it sounds super promising. good job..

Contributor

arthurnn commented Jun 22, 2013

@cheald it sounds super promising. good job..

@cheald

This comment has been minimized.

Show comment
Hide comment
@cheald

cheald Jun 22, 2013

Member

Here's runtimes for HEAD vs my patches:

$ MONGOID_ENV=production bundle exec ruby mongo_orm_benchmark.rb -Xcompile.invokedynamic=true

HEAD (jruby 1.7.4)

MongoMapper Read 150        batches of 1            2.550000   0.030000   2.580000 (  1.539000)
    Mongoid Read 150        batches of 1            1.130000   0.030000   1.160000 (  0.714000)
MongoMapper Read 3750       batches of 25           8.330000   0.140000   8.470000 (  5.578000)
    Mongoid Read 3750       batches of 25           1.790000   0.030000   1.820000 (  1.116000)
MongoMapper Read 15000      batches of 100         12.430000   0.360000  12.790000 (  9.868000)
    Mongoid Read 15000      batches of 100          2.340000   0.030000   2.370000 (  1.420000)
MongoMapper Read 75000      batches of 500         35.190000   1.530000  36.720000 ( 36.695000)
    Mongoid Read 75000      batches of 500          2.210000   0.040000   2.250000 (  2.272000)
MongoMapper Read 150000     batches of 1000        68.900000   3.030000  71.930000 ( 72.076000)
    Mongoid Read 150000     batches of 1000         4.090000   0.040000   4.130000 (  4.178000)

Mine

       user     system      total        real
MongoMapper Read 150        batches of 1           1.630000   0.040000   1.670000 (  0.945000)
    Mongoid Read 150        batches of 1           1.290000   0.030000   1.320000 (  0.763000)
MongoMapper Read 3750       batches of 25          4.430000   0.130000   4.560000 (  2.770000)
    Mongoid Read 3750       batches of 25          1.850000   0.030000   1.880000 (  1.191000)
MongoMapper Read 15000      batches of 100         4.970000   0.350000   5.320000 (  3.236000)
    Mongoid Read 15000      batches of 100         2.680000   0.030000   2.710000 (  1.567000)
MongoMapper Read 75000      batches of 500         6.550000   1.470000   8.020000 (  8.039000)
    Mongoid Read 75000      batches of 500         2.200000   0.030000   2.230000 (  2.258000)
MongoMapper Read 150000     batches of 1000       12.790000   2.860000  15.650000 ( 15.675000)
    Mongoid Read 150000     batches of 1000        4.080000   0.020000   4.100000 (  4.128000)

I don't have any experience profiling under JRuby, and while I'm certainly happy with a ~5x speedup, I want to get it much closer to what Mongoid can do.

Member

cheald commented Jun 22, 2013

Here's runtimes for HEAD vs my patches:

$ MONGOID_ENV=production bundle exec ruby mongo_orm_benchmark.rb -Xcompile.invokedynamic=true

HEAD (jruby 1.7.4)

MongoMapper Read 150        batches of 1            2.550000   0.030000   2.580000 (  1.539000)
    Mongoid Read 150        batches of 1            1.130000   0.030000   1.160000 (  0.714000)
MongoMapper Read 3750       batches of 25           8.330000   0.140000   8.470000 (  5.578000)
    Mongoid Read 3750       batches of 25           1.790000   0.030000   1.820000 (  1.116000)
MongoMapper Read 15000      batches of 100         12.430000   0.360000  12.790000 (  9.868000)
    Mongoid Read 15000      batches of 100          2.340000   0.030000   2.370000 (  1.420000)
MongoMapper Read 75000      batches of 500         35.190000   1.530000  36.720000 ( 36.695000)
    Mongoid Read 75000      batches of 500          2.210000   0.040000   2.250000 (  2.272000)
MongoMapper Read 150000     batches of 1000        68.900000   3.030000  71.930000 ( 72.076000)
    Mongoid Read 150000     batches of 1000         4.090000   0.040000   4.130000 (  4.178000)

Mine

       user     system      total        real
MongoMapper Read 150        batches of 1           1.630000   0.040000   1.670000 (  0.945000)
    Mongoid Read 150        batches of 1           1.290000   0.030000   1.320000 (  0.763000)
MongoMapper Read 3750       batches of 25          4.430000   0.130000   4.560000 (  2.770000)
    Mongoid Read 3750       batches of 25          1.850000   0.030000   1.880000 (  1.191000)
MongoMapper Read 15000      batches of 100         4.970000   0.350000   5.320000 (  3.236000)
    Mongoid Read 15000      batches of 100         2.680000   0.030000   2.710000 (  1.567000)
MongoMapper Read 75000      batches of 500         6.550000   1.470000   8.020000 (  8.039000)
    Mongoid Read 75000      batches of 500         2.200000   0.030000   2.230000 (  2.258000)
MongoMapper Read 150000     batches of 1000       12.790000   2.860000  15.650000 ( 15.675000)
    Mongoid Read 150000     batches of 1000        4.080000   0.020000   4.100000 (  4.128000)

I don't have any experience profiling under JRuby, and while I'm certainly happy with a ~5x speedup, I want to get it much closer to what Mongoid can do.

@cheald

This comment has been minimized.

Show comment
Hide comment
@cheald

cheald Jun 24, 2013

Member

Welp

I'd say that's a win.

I have a couple of bugfixes to this to push. I've also converted the spec suite to RSpec, which resolves the various issues running this test suite on the latest versions of Rails. shoulda + mocha is basically rspec already; a little bit of search-and-replace and some massaging made it work just fine. I can back that out if it's unwelcome, but test suites that run > test suites that don't run, IMO. :)

Member

cheald commented Jun 24, 2013

Welp

I'd say that's a win.

I have a couple of bugfixes to this to push. I've also converted the spec suite to RSpec, which resolves the various issues running this test suite on the latest versions of Rails. shoulda + mocha is basically rspec already; a little bit of search-and-replace and some massaging made it work just fine. I can back that out if it's unwelcome, but test suites that run > test suites that don't run, IMO. :)

@cheald

This comment has been minimized.

Show comment
Hide comment
@cheald

cheald Jun 25, 2013

Member

It's been far too long a road, but everything is now building successfully on Travis, against the latest version of Rails 3.0, 3.1, and 3.2. Next up, Rails 4! :D

Member

cheald commented Jun 25, 2013

It's been far too long a road, but everything is now building successfully on Travis, against the latest version of Rails 3.0, 3.1, and 3.2. Next up, Rails 4! :D

@gaffneyc

This comment has been minimized.

Show comment
Hide comment
@gaffneyc

gaffneyc Jun 26, 2013

Trying out your branch on some code today and this method conflicts with on we have on a model. Would it be worth naming in such a way to reduce possible conflicts?

Trying out your branch on some code today and this method conflicts with on we have on a model. Would it be worth naming in such a way to reduce possible conflicts?

This comment has been minimized.

Show comment
Hide comment
@cheald

cheald Jun 26, 2013

Owner

Yeah, definitely, will do. I'll push a fix shortly; I've got one other bug related to Rails' form_for not prefilling correctly with this branch. I'll push those changes together shortly.

Owner

cheald replied Jun 26, 2013

Yeah, definitely, will do. I'll push a fix shortly; I've got one other bug related to Rails' form_for not prefilling correctly with this branch. I'll push those changes together shortly.

This comment has been minimized.

Show comment
Hide comment
@gaffneyc

gaffneyc Jun 26, 2013

Looking at the code it doesn't look like this is use anywhere outside of initialize_default_values. Would it be worth just moving the logic in there?

And wanted to say: great work! Excited to try this out in our app.

gaffneyc replied Jun 26, 2013

Looking at the code it doesn't look like this is use anywhere outside of initialize_default_values. Would it be worth just moving the logic in there?

And wanted to say: great work! Excited to try this out in our app.

This comment has been minimized.

Show comment
Hide comment
@cheald

cheald Jun 26, 2013

Owner

Fixes pushed. Thanks for reviewing, and please let me know how the experiment goes!

Owner

cheald replied Jun 26, 2013

Fixes pushed. Thanks for reviewing, and please let me know how the experiment goes!

@gaffneyc

This comment has been minimized.

Show comment
Hide comment
@gaffneyc

gaffneyc Jun 26, 2013

Nitpick:

def initialize_default_values(exclusions = {})
  # Init the keys ivar. Due to the volume of times this method is called, we don't want it in a method.
  @keys = self.class.keys

  exclusions ||= {}
  self.class.default_keys.each do |key|
    if exclusions.key?(key)
      internal_write_key(key.name, key.default_value, false)
    end
  end
end

I think if is wrapping the code easier to grok than doing a next.

Nitpick:

def initialize_default_values(exclusions = {})
  # Init the keys ivar. Due to the volume of times this method is called, we don't want it in a method.
  @keys = self.class.keys

  exclusions ||= {}
  self.class.default_keys.each do |key|
    if exclusions.key?(key)
      internal_write_key(key.name, key.default_value, false)
    end
  end
end

I think if is wrapping the code easier to grok than doing a next.

@jakolehm

This comment has been minimized.

Show comment
Hide comment
@jakolehm

jakolehm Jun 28, 2013

Looks really nice, good job!

jakolehm commented Jun 28, 2013

Looks really nice, good job!

@jcaudle

This comment has been minimized.

Show comment
Hide comment
@jcaudle

jcaudle Jul 2, 2013

Contributor

@cheald, thanks for the submission. It's going to take me a while to get through something this big. Do you think you could get it split into a few different pull requests as there's so much going on here? I'll still be looking at this, but I'm not sure how I'll go about determining how we'll want to bring this into the project as so many things change in this.

Contributor

jcaudle commented Jul 2, 2013

@cheald, thanks for the submission. It's going to take me a while to get through something this big. Do you think you could get it split into a few different pull requests as there's so much going on here? I'll still be looking at this, but I'm not sure how I'll go about determining how we'll want to bring this into the project as so many things change in this.

@cheald

This comment has been minimized.

Show comment
Hide comment
@cheald

cheald Jul 2, 2013

Member

I'll see what I can do - I know that it's quite the massive changeset. I've continued development on it in a different branch, as well, which includes better ActiveRecord parity and a little more integration with ActiveModel so that Rails version-specific behaviors are more consistent with AR behaviors for those versions.

The really big change in terms of "lines changed" is the addition of the rspec test suite, which I did since the current test suite wouldn't pass on multiple Rails versions due to Mocha version conflicts between versions of Rails. I didn't remove the test/unit suite, just added the rspec suite (which is a literal copy-paste-and-fix of the TU suite). I'll see if I can factor that out, but i'm not sure how much I'll be able to extract by itself, since there are numerous changes that depend on the changes before them.

For what it's worth, I've been running this branch in production on mashable.com for the past 10 days and it's performing like a champ. :)

Member

cheald commented Jul 2, 2013

I'll see what I can do - I know that it's quite the massive changeset. I've continued development on it in a different branch, as well, which includes better ActiveRecord parity and a little more integration with ActiveModel so that Rails version-specific behaviors are more consistent with AR behaviors for those versions.

The really big change in terms of "lines changed" is the addition of the rspec test suite, which I did since the current test suite wouldn't pass on multiple Rails versions due to Mocha version conflicts between versions of Rails. I didn't remove the test/unit suite, just added the rspec suite (which is a literal copy-paste-and-fix of the TU suite). I'll see if I can factor that out, but i'm not sure how much I'll be able to extract by itself, since there are numerous changes that depend on the changes before them.

For what it's worth, I've been running this branch in production on mashable.com for the past 10 days and it's performing like a champ. :)

@cheald

This comment has been minimized.

Show comment
Hide comment
@cheald

cheald Jul 2, 2013

Member

See also, #526.

Okay, I think I've gotten everything split out and squashed as appropriate. The only things missing are the Rails 4 compatibility and associated specs, and the specs for the embedded callbacks change, since those depend on the rspec suite.

If we can get these merged, then I can submit another PR for my Rails 4 compatibility fixes. I don't have my app booting against Rails 4 yet, but MM's specs run against it just dandy, and I've fixed things like the attr_protected/attr_accessible syntax to follow 4's preferences.

Member

cheald commented Jul 2, 2013

See also, #526.

Okay, I think I've gotten everything split out and squashed as appropriate. The only things missing are the Rails 4 compatibility and associated specs, and the specs for the embedded callbacks change, since those depend on the rspec suite.

If we can get these merged, then I can submit another PR for my Rails 4 compatibility fixes. I don't have my app booting against Rails 4 yet, but MM's specs run against it just dandy, and I've fixed things like the attr_protected/attr_accessible syntax to follow 4's preferences.

@cheald

This comment has been minimized.

Show comment
Hide comment
@cheald

cheald Jul 2, 2013

Member

The individual pulls don't pass CI, but together they do: https://travis-ci.org/cheald/mongomapper/builds/8650187

That's a branch consisting of master with load_speed_fixes, rspec_suite, and embedded_callback_fix merged in.

Member

cheald commented Jul 2, 2013

The individual pulls don't pass CI, but together they do: https://travis-ci.org/cheald/mongomapper/builds/8650187

That's a branch consisting of master with load_speed_fixes, rspec_suite, and embedded_callback_fix merged in.

@jnunemaker

This comment has been minimized.

Show comment
Hide comment
@jnunemaker

jnunemaker Jul 3, 2013

Contributor

Is there a particular order all these pulls need to be merged in?

Contributor

jnunemaker commented Jul 3, 2013

Is there a particular order all these pulls need to be merged in?

@cheald

This comment has been minimized.

Show comment
Hide comment
@cheald

cheald Jul 3, 2013

Member

No, but in my merge_all branch I merged in order: load_speed_fixes, rspec_suite, embedded_callback_fix. I have Rails 4.0 support and some additional specs in massive_performance_overhaul that I can extract and submit a PR for, but they depend on the first two branches so I didn't want to submit PRs with hard dependencies.

Member

cheald commented Jul 3, 2013

No, but in my merge_all branch I merged in order: load_speed_fixes, rspec_suite, embedded_callback_fix. I have Rails 4.0 support and some additional specs in massive_performance_overhaul that I can extract and submit a PR for, but they depend on the first two branches so I didn't want to submit PRs with hard dependencies.

@jnunemaker

This comment has been minimized.

Show comment
Hide comment
@jnunemaker

jnunemaker Jul 3, 2013

Contributor

Nope, no more PR's. :) I'm working on getting these pulled now. How about I give you the keys and if all looks good on future pulls you can merge them yourself. :)

Contributor

jnunemaker commented Jul 3, 2013

Nope, no more PR's. :) I'm working on getting these pulled now. How about I give you the keys and if all looks good on future pulls you can merge them yourself. :)

@cheald

This comment has been minimized.

Show comment
Hide comment
@cheald

cheald Jul 3, 2013

Member

I'd love to become an official maintainer of the project. I'm heavily invested in it, and like to think that I know it pretty well! :)

Member

cheald commented Jul 3, 2013

I'd love to become an official maintainer of the project. I'm heavily invested in it, and like to think that I know it pretty well! :)

@jnunemaker

This comment has been minimized.

Show comment
Hide comment
@jnunemaker

jnunemaker Jul 3, 2013

Contributor

Done. I really appreciate the help. Are you on the mailing list? I had mentioned looking for other people to help there a while back. Not sure if you saw that.

Contributor

jnunemaker commented Jul 3, 2013

Done. I really appreciate the help. Are you on the mailing list? I had mentioned looking for other people to help there a while back. Not sure if you saw that.

@jcaudle

This comment has been minimized.

Show comment
Hide comment
@jcaudle

jcaudle Jul 3, 2013

Contributor

Welcome @cheald!

Contributor

jcaudle commented Jul 3, 2013

Welcome @cheald!

@cheald

This comment has been minimized.

Show comment
Hide comment
@cheald

cheald Jul 3, 2013

Member

I'm not and should be! I've fixed that. Thank you!

Member

cheald commented Jul 3, 2013

I'm not and should be! I've fixed that. Thank you!

@jnunemaker

This comment has been minimized.

Show comment
Hide comment
@jnunemaker

jnunemaker Jul 3, 2013

Contributor

@cheald would you mind getting these all merged into master and the specs passing? Feel free to kill the old test folder if you are sure you got everything. I'm still getting some failures, like maybe a merge didn't go right.

Contributor

jnunemaker commented Jul 3, 2013

@cheald would you mind getting these all merged into master and the specs passing? Feel free to kill the old test folder if you are sure you got everything. I'm still getting some failures, like maybe a merge didn't go right.

@cheald

This comment has been minimized.

Show comment
Hide comment
@cheald

cheald Jul 3, 2013

Member

Sure, I'd be happy to.

Member

cheald commented Jul 3, 2013

Sure, I'd be happy to.

@jnunemaker

This comment has been minimized.

Show comment
Hide comment
@jnunemaker

jnunemaker Jul 3, 2013

Contributor

@cheald I also fixed the plucky issue and released a new version.

Contributor

jnunemaker commented Jul 3, 2013

@cheald I also fixed the plucky issue and released a new version.

@cheald

This comment has been minimized.

Show comment
Hide comment
@cheald

cheald Jul 3, 2013

Member

Fantastic. Can we bump the minimum version in the gemspec for the next MM release, so that people updating get the newest Plucky, too?

Member

cheald commented Jul 3, 2013

Fantastic. Can we bump the minimum version in the gemspec for the next MM release, so that people updating get the newest Plucky, too?

@cheald

This comment has been minimized.

Show comment
Hide comment
@cheald

cheald Jul 3, 2013

Member

@jnunemaker Any objection to me merging in the straggler commits from this branch? They basically just add Rails 4 and Ruby 2 testing to Travis and fix minor things so that the suite runs on Rails 4. It doesn't include the actually meaty Rails 4 changes, but it goes get the spec suite building.

Member

cheald commented Jul 3, 2013

@jnunemaker Any objection to me merging in the straggler commits from this branch? They basically just add Rails 4 and Ruby 2 testing to Travis and fix minor things so that the suite runs on Rails 4. It doesn't include the actually meaty Rails 4 changes, but it goes get the spec suite building.

@jnunemaker

This comment has been minimized.

Show comment
Hide comment
@jnunemaker

jnunemaker Jul 3, 2013

Contributor

Yep.

On Jul 3, 2013, at 4:35 PM, Chris Heald notifications@github.com wrote:

Fantastic. Can we bump the minimum version in the gemspec for the next MM release, so that people updating get the newest Plucky, too?


Reply to this email directly or view it on GitHub.

Contributor

jnunemaker commented Jul 3, 2013

Yep.

On Jul 3, 2013, at 4:35 PM, Chris Heald notifications@github.com wrote:

Fantastic. Can we bump the minimum version in the gemspec for the next MM release, so that people updating get the newest Plucky, too?


Reply to this email directly or view it on GitHub.

@jnunemaker

This comment has been minimized.

Show comment
Hide comment
@jnunemaker

jnunemaker Jul 3, 2013

Contributor

Sounds fine to me.

On Jul 3, 2013, at 4:55 PM, Chris Heald notifications@github.com wrote:

@jnunemaker Any objection to me merging in the straggler commits from this branch? They basically just add Rails 4 and Ruby 2 testing to Travis and fix minor things so that the suite runs on Rails 4. It doesn't include the actually meaty Rails 4 changes, but it goes get the spec suite building.


Reply to this email directly or view it on GitHub.

Contributor

jnunemaker commented Jul 3, 2013

Sounds fine to me.

On Jul 3, 2013, at 4:55 PM, Chris Heald notifications@github.com wrote:

@jnunemaker Any objection to me merging in the straggler commits from this branch? They basically just add Rails 4 and Ruby 2 testing to Travis and fix minor things so that the suite runs on Rails 4. It doesn't include the actually meaty Rails 4 changes, but it goes get the spec suite building.


Reply to this email directly or view it on GitHub.

@cheald cheald closed this Jul 3, 2013

@cheald

This comment has been minimized.

Show comment
Hide comment
@cheald

cheald Jul 3, 2013

Member

Closed via merges of other branches.

Member

cheald commented Jul 3, 2013

Closed via merges of other branches.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment