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

Adding Mongodb Driver #1

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

Adding Mongodb Driver #1

wants to merge 9 commits into from

Conversation

meabed
Copy link

@meabed meabed commented Dec 27, 2017

  • Adding Mongodb Driver
  • Update Readme to include Mongodb

- Update Readme to include Mongodb
@meabed
Copy link
Author

meabed commented Dec 27, 2017

Awesome project Thank you.
Small contribution to the project, let me know if it need any changes or fixes.

@meabed
Copy link
Author

meabed commented Dec 27, 2017

screen shot 2017-12-27 at 11 47 34 pm

@jhuckaby
Copy link
Owner

Thank you for this! I have never used MongoDB before, so I will have to learn a bit about it and install a local version, so I can test this. If I merge this PR, I have to be able to support it going forward, and for that I have to understand how Mongo works.

I'm a bit concerned about converting all binary records to base64 -- is this really the only way to do it with Mongo? That seems extremely wasteful. I have applications that store 100MB binary records, for example.

Why are we creating an index on every startup? Isn't this incorrect? I am referring to this line:

self.collection.createIndex({key: 1}, function (err, result) {

Maybe I just don't understand MongoDB, but it seems like the index should only need to be created on the collection one time only, when the collection is first created.

Small typo in your docs addition for serialize here:

When set to `true` (the default), this is left up to Mongodb to handle.

This logic is reversed. When serialize is false, it sends the object directly to Mongo for it to do the serialization. When serialize is true, we perform the serialization in Node.js first.

I might want to change the name of the module from Mongodb to MongoDB, to better match their branding.

Thanks again!

@meabed
Copy link
Author

meabed commented Dec 30, 2017

Thanks @jhuckaby for the review :)

No worries get sometime to use mongodb =)

  • Binary conversation is fixed :) it store files as binary data Mongo collection file size limit 16MB more.

  • To address the file size limits, I have added GridFS for large object / files storage in MongoDB more

  • createIndex to be able to do fast lookups
    Creates an index on the specified field if the index does not already exist more.

  • serialize is fixed.

  • Engine name changed to MongoDB

  • I have added same test file for mongodb with configuration and ran both tests below.

Thanks 👍 let me know if you got chance to test it :)

screen shot 2017-12-31 at 3 14 04 am

The reason this driver is helpful is MongoDB is very popular and its 1 minute to get it up and running in docker container!

I have tried the Couchbase configuration but it took me ages to get it up :) and yet couldn't figure out many things :) must be me because i'm know to it, but nevertheless the more drivers the better :)

@meabed
Copy link
Author

meabed commented Jan 17, 2018

ping :)

@meabed
Copy link
Author

meabed commented Jan 24, 2018

@jhuckaby have you had a chance to look the PR :) anything needed i can do to help getting it tested and merged?

Thanks

@jhuckaby
Copy link
Owner

Hello. I am very sorry, but I have not had time to test this, and am not ready to merge the PR yet. I have never used MongoDB in my entire life, so I am completely ignorant to it. If I am to merge this PR in with the base code, I have to understand how to install it, and use it, and test it. This will take some time. I apologize, but I will get to this as soon as I can. Thank you again for the great PR!

@meabed
Copy link
Author

meabed commented Jan 25, 2018

no worries :) let me know if i could be in any help!

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.

2 participants