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

Mongodb support #117

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Mongodb support #117

wants to merge 10 commits into from

Conversation

coderarity
Copy link
Contributor

Hey, I added mongodb support!

It includes tests that use localhost mongodb, so you have to make sure to run mongod in some other terminal before running the tests. The engine passes all of the tests.

It does some funny stuff involving checking for a fully connected client before running a command so that it can run some setup logic first. Most of that is in the request and setup functions. It'd be nice if we could move this into resourceful somehow, there must be some other engine that has initialization logic as well that is struggling.

There's a mongodb-support-2 branch which was pushed by accident in an attempt to not force-push to mongodb-support, which I ended up doing anyways. (It was because I ran git rebase master on the mongodb-support branch). In any case, I checked to make sure it'd easily rebase onto master, so all that should need to be done is git rebase mongodb-support to integrate this. Github seems to be fine with it.

Anyways, thanks!

@travisbot
Copy link

This pull request fails (merged 83b174b into ff957d1).

@travisbot
Copy link

This pull request passes (merged 1de2af8 into ff957d1).

@coderarity
Copy link
Contributor Author

Hey travis, thanks bro!

@Marak
Copy link
Contributor

Marak commented Sep 6, 2012

@coderarity - The process is hanging when trying to run the basic examples. For instance:

var resourceful = require('../lib/resourceful');
resourceful.use('mongodb');

var Author = resourceful.define('author');
Author.string('name');

Author.create({
  id: 'Marak'
}, function(err, marak){
  console.log(marak);
});

Outputs:

node examples/create.js 
{ id: 'Marak', name: undefined, resource: 'Author' }

But then just hangs and the process never closes. Any ideas?

@coderarity
Copy link
Contributor Author

I see. I think mongodb needs to close the database before exiting, but there's nothing to support that in resourceful. Any idea how I can go about closing the mongodb client?

@Marak
Copy link
Contributor

Marak commented Sep 7, 2012

The best action might be requiring the user to explicitly call
Resource.disconnect in order to end the process.

I'm not sure of any other logical way it could be handled.

On Thu, Sep 6, 2012 at 5:16 PM, Christian Howe [email protected]:

I see. I think mongodb needs to close the databasehttp://mongodb.github.com/node-mongodb-native/api-generated/db.html#closebefore exiting, but there's nothing to support that in resourceful. Any
idea how I can go about closing the mongodb client?


Reply to this email directly or view it on GitHubhttps://github.com//pull/117#issuecomment-8352081.

@Marak
Copy link
Contributor

Marak commented Sep 7, 2012

I looked into how mongoose handles this, and it appears they are using a state machine to keep track of all connections and disconnections.

I'm not too keen on refactoring the way resourceful handles engine connections right now.

@coderarity - Have you done any testing to see how connection pooling is handled with the mongodb module being used and how we are using it with this new engine?

I'm OK with the examples hanging for now and the connection not closing, but we need to make sure that connection pooling is happening for outgoing requests. The concern here would be getting into a situation where every new resource instance or resource method call would invoke an additional connection to the database, instead of intelligently sharing / pooling connections.

@cgarvis
Copy link

cgarvis commented Oct 12, 2012

Any chance to get this merged in?

@coderarity
Copy link
Contributor Author

yeah, I'll get it merged in soon, we have to make some changes based on what marak was saying.

@Lipathor
Copy link

Lipathor commented Nov 6, 2012

Is it possible to use default mongo-style id with that engine?

@ericchaves
Copy link
Contributor

Hi @coderarity,

It seems that there's being some change in the way mongodb driver is called. When I try to use the mongodb branch I apparently no connection is made and the following message appears:

========================================================================================
=  Please ensure that you set the default safe variable to one of the                  =
=   allowed values of [true | false | {j:true} | {w:n, wtimeout:n} | {fsync:true}]     =
=   the default value is false which means the driver receives does not                =
=   return the information of the success/error of the insert/update/remove            =
=                                                                                      =
=   ex: new Db(new Server('localhost', 27017), {safe:false})                           =
=                                                                                      =
=   http://www.mongodb.org/display/DOCS/getLastError+Command                           =
=                                                                                      =
=  The default of false will change to true in the near future                         =
=                                                                                      =
=  This message will disappear when the default safe is set on the driver Db           =
========================================================================================

Also, is there plans to merge this engine into the master branch?

Cheers,

Eric.

@mmalecki
Copy link
Contributor

I like the idea, considering that people still want to use MongoDB. In my opinion, however, this should be a seperate module.

@mmalecki
Copy link
Contributor

Oh, and as to the disconnection problem - we could possibly unref the socket it's using. I'll try and see if that's possible.

@pvorb
Copy link

pvorb commented Feb 28, 2013

@mmalecki I think, CouchDB should also be a separate module. So you can plug-in only what one you need.

@imbroglioj
Copy link

On the issue of closing mongodb: my understanding is that the Node approach is to use one mongoDB session everywhere. That can be accomplished simply in resourceful/mongodb by handling a property in the config argument to MongoDb creation:
'db: MY_OPEN_MONGODB_SESSION'
I have done that in my local version and it is working in my unit tests to prevent leaking connections. Let me know if you foresee any problems or if you want me to offer any more information.

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.

None yet

9 participants