Mongoid update #98

sujal · 2011-12-24T05:05:20Z

Hi - Can i have someone take a look at this set of changes and review the approach?

I've basically updated the em-mongo support to cover more functionality, and I've then used that to update mongoid support to use em-mongo inside synchrony.

My goal is to get mongoid to use the connection pool that's part of synchrony and em-mongo, since that seems to be the preferred stack for synchrony and, thus, Goliath.

I'm still writing tests and will probably find things that aren't quite right, so this isn't ready to be merged yet. Thought this would be the easiest way to get a conversation going since there isn't a mailing list.

I'll be testing this for use in a production Goliath app.

(one additional note: The changes to break some compatibility with the existing em-mongo functionality to get it closer to the original non-synchrony em-mongo and mongo-ruby-driver.)

Thanks,

Sujal

The goal is to match the original mongo-ruby-driver so that mongoid cna use em-mongo instead of the socket hack. removing unecessary commented out requires

wait until we have confirmation from others

still working on full testing regimen

danhealy · 2011-12-24T06:12:27Z

Awesome!

mostlyobvious · 2011-12-24T09:39:48Z

I can also share my gist of tweaks to have mongoid use em-mongo, but this is quite simillar. One downside of this is that mongoid would still use one connection regardless of synchrony pool size.

sujal · 2011-12-24T14:14:04Z

I'd love to see it if you are able to post it.

As for the single connection, are you saying that the code I posted will only use a single connection? Can you elaborate? I'd like to look into fixing that.

Sujal

On Dec 24, 2011, at 4:39 AM, Paweł [email protected] wrote:

I can also share my gist of tweaks to have mongoid use em-mongo, but this is quite simillar. One downside of this is that mongoid would still use one connection regardless of synchrony pool size.

Reply to this email directly or view it on GitHub:
#98 (comment)

paneq · 2011-12-24T14:22:11Z

Mongoid caches connections per class. I will provide an example soon.

sujal · 2011-12-24T18:11:22Z

Ah, ok, just looked through that code. Is it caching the connection or what's returned from the master call? In that case, wouldn't it be caching the connection pool per class?

I'll test this all out soon. Just need to fix some bugs so my test server starts up.

Sujal

On Dec 24, 2011, at 9:22 AM, Robert [email protected] wrote:

Mongoid caches connections per class. I will provide an example soon.

Reply to this email directly or view it on GitHub:
#98 (comment)

paneq · 2011-12-26T16:33:23Z

Unfortunately it is not caching a pool but one obtained connection from Fiber pool. So in most cases all you classes will end up caching and thus using later a first connection from the Pool. Different fibers queue requests to the same connection so it is almost like synchronously waiting for execution. It is possible that one Fiber grabs one connection from the pool, is yielded, and different fiber grabs a different connection for another class when executing its code. That way different classes will end up using different connection under the hood. Effective working with pool is only possible when directly executing queries using the driver.

Chat.collection.driver
=> #<EventMachine::Mongo::Collection:0x00000003922248 @db="messenger_development", @ns="messenger_chats", @name="messenger_development.messenger_chats", @connection=#<EventMachine::Mongo::Connection:0x00000003c5ad48 @em_connection=#<EventMachine::Mongo::EMConnection:0x00000003c5abb8 @signature=21, @request_id=2, @retries=0, @responses={}, @is_connected=true, @host="127.0.0.1", @port=27017, @on_unbind=#<Proc:0x00000003c5aa50@/home/rupert/.rvm/gems/ruby-1.9.2-p180-fastrequire/bundler/gems/em-mongo-392c86c71a6f/lib/em-mongo/connection.rb:116>, @reconnect_in=false, @slave_ok=false, @on_close=#<Proc:0x00000003c5aa00@/home/rupert/.rvm/gems/ruby-1.9.2-p180-fastrequire/bundler/gems/em-mongo-392c86c71a6f/lib/em-mongo/connection.rb:120>, @deferred_status=:succeeded, @errbacks=[], @buffer=, @deferred_timeout=nil, @callbacks=nil, @deferred_args=[]>, @db={"messenger_development"=>#<EventMachine::Mongo::Database:0x00000003c5a6b8 @db_name="messenger_development", @em_connection=#<EventMachine::Mongo::Connection:0x00000003c5ad48 ...>, @collection=nil, @collections={}, @cache_time=300>}>>

Configuration:

Mongoid.configure do |c|
  c.max_retries_on_connection_failure = 0
  c.skip_version_check = true
  c.identity_map_enabled = false
  c.database = EM::Synchrony::ConnectionPool.new(:size => config[:pool].to_i) do
    EM::Mongo::Connection.new.db(config[:database])
  end

igrigorik · 2011-12-27T16:11:22Z

lib/em-synchrony/em-mongo.rb

+        name_resp.callback do |docs|
+          names = docs.collect{ |doc| doc['name'] || '' }
+          names = names.delete_if {|name| name.index(self.name).nil? || name.index('$')}
+          names = names.map{ |name| name.sub(self.name + '.','')}


Why do we need all this extra processing here? Shouldn't we stick to drivers default response?

I didn't have a better idea yet - basically, they have deep calls to other functions that have been made sync, so my choices were to redo these methods to call the async version or come up with another solution. (the main changes are the changes to call the adefer_as_a or the a* methods (in the first method).

Basically, what you're seeing is my attempt to get this working, at which point I'll step back and then refactor to clean things up, eliminate the copy/pasting and brute force stuff. For this one, though, I don't have a better idea yet.

igrigorik · 2011-12-27T16:20:38Z

Quick additional note: can we also add insert command to the list? It's not actually implemented and we've had a few instances already of people bumping heads with it..

Also, let's drop old driver support, it'll make everyone's life easier (yes, laggards will have to upgrade :))

sujal · 2011-12-27T16:25:47Z

The good thing is most of what you pointed out is on my list of stuff to fix. There's a bit of cargo culting (though I did read all of it) i did to save time.

Will take a look at everything in detail once I'm back on this task this afternoon.

I'm working on getting a reliable authentication call working - the way I have it now, the auth call causes a stack too deep error. I have a lead on that, hope to have it sorted today.

Sujal

On Dec 27, 2011, at 11:20 AM, Ilya [email protected] wrote:

Quick additional note: can we also add insert command to the list? It's not actually implemented and we've had a few instances already of people bumping heads with it..

Also, let's drop old driver support, it'll make everyone's life easier (yes, laggards will have to upgrade :))

Reply to this email directly or view it on GitHub:
#98 (comment)

pkieltyka · 2011-12-27T20:26:47Z

+1 .. I'd love to see mongoid+em-mongo

needed to do this to get authentication working properly in a connection pool's block.

Not really sure why em-mongo does this - I assume it's a flag to indicate end of result set? No idea, but it's breaking Mongoid, so a quick patch

em-mongo insists that the document ID be :_id (a Symbol), not a "_id" (String) Will submit issue to em-mongo to ask if they should match the standard ruby driver's behavior which doesn't assume.

might be time to break this out into a separate project?

missing or misdirected aliases were the culprit

insert and update already return real values,

modeled after mongoid tests

includes additional finder tests - just the start

sujal · 2012-01-03T07:50:51Z

OK, now we're ready. Missed one callback case (an obvious one in hindsight, but one my API wasn't using often). :(

It worked, and the next batch process that hammers the API will run in a few minutes. Will have validation that this is working enough for us then.

Sujal

igrigorik · 2012-01-07T21:40:44Z

lib/em-synchrony/em-mongo.rb

+
+        # if selector.keys.length > 1 && RUBY_VERSION < '1.9' && selector.class != BSON::OrderedHash
+        #   raise MongoArgumentError, "DB#command requires an OrderedHash when hash contains multiple keys"
+        # end


If we don't need it, let's drop the dead code..

igrigorik · 2012-01-07T21:52:04Z

A few pedantic questions / notes above.. plus, there is a bunch of dead code in the spec files - can we remove that?

If the specs are all good, we can merge this and iterate from there.. since it's an almost unwieldy piece of code already.

sujal · 2012-01-08T00:57:14Z

Thanks - most of the comments made sense, will take a closer look tomorrow.

Was talking to someone about this - wondered if it made sense to make this part of em-synchrony directly, or to make it its own gem.

On one hand, the bulk of the work is to the em-mongo driver, which has always been part of em-synchrony. The mongoid tweaks are just around the connections, mostly.

On the other hand, the code does touch a lot of the em-mongo API...

Anyway, something to think about -- I'll have an update on this tomorrow or Monday most likely. I have that connection caching thing to verify and sort out in mongoid on my list, just to make sure that the pool is being used properly.

Sujal

On Jan 7, 2012, at 4:52 PM, Ilya Grigorik wrote:

A few pedantic questions / notes above.. plus, there is a bunch of dead code in the spec files - can we remove that?

If the specs are all good, we can merge this and iterate from there.. since it's an almost unwieldy piece of code already.

Reply to this email directly or view it on GitHub:
#98 (comment)

igrigorik · 2012-01-08T01:59:15Z

Sounds good. For where to keep the code: I'm good either way.. as long as we make sure its supported moving forward. :)

Fixes typos in the finders spec for mongoid

jwarchol · 2012-02-02T22:43:43Z

At this point I don't think the spec problem I'm seeing in this branch is related to the connection pool. I've been talking to Sujal via email and hope to mind meld on this ASAP. At some point in the spec run, a request just doesn't get made and things halt. The reactor is still running, if I put a little "puts '.'" periodic timer in it shows output. But the request never gets made by em-mongo's connection and thing sit. I'm boggled by this, as if I move the specs around I can get the halt to change position, but it happens all the same. Really need some help on this, I think something is amiss in the em-mongo patching.

jwarchol · 2012-02-03T15:18:45Z

Discovered last night by turning mongod logging way up that its having "9001 socket exception" errors that were not making it back through em-mongo in any visible way. Digging in on that front next.

sujal · 2012-02-03T15:21:53Z

Ah, crap - I forgot we were supposed to look at this. I can possibly do this later today, but Monday would be a lot better for me. Shoot me an email off GitHub whenever you have time and let's just pick a time and get it done.

Sujal

On Feb 3, 2012, at 10:18 AM, Joshua Warchol wrote:

Discovered last night by turning mongod logging way up that its having "9001 socket exception" errors that were not making it back through em-mongo in any visible way. Digging in on that front next.

Reply to this email directly or view it on GitHub:
#98 (comment)

This was breaking on nested cursor enumeration, at least on our production service. I don't have a handy test case written yet. Will work on that next.

Conflicts: lib/em-synchrony/mongo.rb

igrigorik · 2012-03-27T04:33:26Z

Hey guys, apologies for dropping the ball on this. How do we look now in terms of completeness and functionality?

lucianopanaro · 2012-03-27T14:27:37Z

Hi Ilya, thanks for picking this up. I will also review the code, run the specs and see how it goes

sujal · 2012-03-27T14:33:05Z

There's still a broken testing setup somewhere in there which points to a bug in the underlying work i did with en-mongo/mongoid. At least, that's what it seems like. One test hangs, but it's always the 4th test to run (even if I reorder them, the 4th test hangs no matter which test it is).

That's as far as I got. Maybe josh has other details to share.

That being said, our API server is running in prod and working with limited traffic just fine.

Sujal

On Mar 27, 2012, at 10:27 AM, Luciano [email protected] wrote:

Hi Ilya, thanks for picking this up. I will also review the code, run the specs and see how it goes

Reply to this email directly or view it on GitHub:
#98 (comment)

jwarchol · 2012-03-27T15:39:29Z

Sorry, I don't have anything more to add beyond what @sujal said. Unless we get someone with really deep Mongoid knowledge involved, I'd consider using it on top of em-mongo synchrony a "cutting edge" option.

lucianopanaro · 2012-03-27T15:42:45Z

I have actually worked with MongoID internals quite a bit so I will give it a try

igrigorik · 2012-07-21T18:55:52Z

@lgs @sujal thanks for staying with this - would love to see this live an working.

By the sounds of it, we should close this pull and open a new bug/thread to keep track?

sujal · 2012-07-21T20:19:27Z

Agreed.

On Jul 21, 2012, at 2:55 PM, Ilya Grigorik [email protected] wrote:

@lgs @sujal thanks for staying with this - would love to see this live an working.

By the sounds of it, we should close this pull and open a new bug/thread to keep track?

Reply to this email directly or view it on GitHub:
#98 (comment)

sujal added 3 commits December 23, 2011 19:34

enhance em-mongo to be more fully sync throughout

89737ba

The goal is to match the original mongo-ruby-driver so that mongoid cna use em-mongo instead of the socket hack. removing unecessary commented out requires

revert the require

81ea17d

wait until we have confirmation from others

mongoid uses em-mongo now

492e3f1

still working on full testing regimen

igrigorik reviewed Dec 27, 2011
View reviewed changes

sujal added 14 commits December 28, 2011 13:51

change connection success symantics to use callback instead of next-tick

ce04cf5

needed to do this to get authentication working properly in a connection pool's block.

handle em-mongo's nil behavior for cursor#each

3e8410a

Not really sure why em-mongo does this - I assume it's a flag to indicate end of result set? No idea, but it's breaking Mongoid, so a quick patch

remove old em-mongo driver support

b379b22

fix require order

6d84441

metaprogram the simple overrides

04bf5d2

fixes serialization bug triggered by em-mongo

55728ed

em-mongo insists that the document ID be :_id (a Symbol), not a "_id" (String) Will submit issue to em-mongo to ask if they should match the standard ruby driver's behavior which doesn't assume.

prepare for larger mongoid suite

b9aa9d8

might be time to break this out into a separate project?

get em-mongo specs to pass

1a4e500

missing or misdirected aliases were the culprit

make safe update and safe_insert sync

9ddbd12

insert and update already return real values,

beginning test refactor to test basic mongoid functionality

6c5c03d

adds a few robustness checks to fiber callbacks

9ef6586

fix require paths

e4411fa

initial functionality testing

6e20bbb

modeled after mongoid tests

fixes to finder logic (return values were wrong)

fe50422

includes additional finder tests - just the start

sujal added 2 commits January 3, 2012 01:14

deleting unnecessary code

2916e60

found the missing fiber wrapper

b59bc67

removing DS_Store

75ca839

igrigorik reviewed Jan 7, 2012
View reviewed changes

sujal and others added 3 commits January 8, 2012 15:14

code cleanup from comments on pull request

f7bc20e

Fixed syntax error in mongoid finders spec

98664da

Merge pull request #1 from jwarchol/mongoid-update

2b7c923

Fixes typos in the finders spec for mongoid

sujal added 5 commits February 21, 2012 21:00

looks to be a missing resume here

1d866bd

simplify .each

afd6eb8

This was breaking on nested cursor enumeration, at least on our production service. I don't have a handy test case written yet. Will work on that next.

add a to_a and ato_a method to pass tests

2f5f123

Merge branch 'master' into mongoid-update

66e3cbd

Conflicts: lib/em-synchrony/mongo.rb

fix the require order, simplify

fc86b23

igrigorik closed this Jul 21, 2012

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mongoid update #98

Mongoid update #98

sujal commented Dec 24, 2011

danhealy commented Dec 24, 2011

mostlyobvious commented Dec 24, 2011

sujal commented Dec 24, 2011

paneq commented Dec 24, 2011

sujal commented Dec 24, 2011

paneq commented Dec 26, 2011

igrigorik Dec 27, 2011

sujal Dec 28, 2011

igrigorik commented Dec 27, 2011

sujal commented Dec 27, 2011

pkieltyka commented Dec 27, 2011

sujal commented Jan 3, 2012

igrigorik Jan 7, 2012

sujal Jan 8, 2012

igrigorik commented Jan 7, 2012

sujal commented Jan 8, 2012

igrigorik commented Jan 8, 2012

jwarchol commented Feb 2, 2012

jwarchol commented Feb 3, 2012

sujal commented Feb 3, 2012

igrigorik commented Mar 27, 2012

lucianopanaro commented Mar 27, 2012

sujal commented Mar 27, 2012

jwarchol commented Mar 27, 2012

lucianopanaro commented Mar 27, 2012

igrigorik commented Jul 21, 2012

sujal commented Jul 21, 2012

Mongoid update #98

Mongoid update #98

Conversation

sujal commented Dec 24, 2011

danhealy commented Dec 24, 2011

mostlyobvious commented Dec 24, 2011

sujal commented Dec 24, 2011

paneq commented Dec 24, 2011

sujal commented Dec 24, 2011

paneq commented Dec 26, 2011

igrigorik Dec 27, 2011

Choose a reason for hiding this comment

sujal Dec 28, 2011

Choose a reason for hiding this comment

igrigorik commented Dec 27, 2011

sujal commented Dec 27, 2011

pkieltyka commented Dec 27, 2011

sujal commented Jan 3, 2012

igrigorik Jan 7, 2012

Choose a reason for hiding this comment

sujal Jan 8, 2012

Choose a reason for hiding this comment

igrigorik commented Jan 7, 2012

sujal commented Jan 8, 2012

igrigorik commented Jan 8, 2012

jwarchol commented Feb 2, 2012

jwarchol commented Feb 3, 2012

sujal commented Feb 3, 2012

igrigorik commented Mar 27, 2012

lucianopanaro commented Mar 27, 2012

sujal commented Mar 27, 2012

jwarchol commented Mar 27, 2012

lucianopanaro commented Mar 27, 2012

igrigorik commented Jul 21, 2012

sujal commented Jul 21, 2012