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

Integrated mongodb driver code #112

Merged
merged 5 commits into from
Apr 8, 2013
Merged

Integrated mongodb driver code #112

merged 5 commits into from
Apr 8, 2013

Conversation

christkv
Copy link
Contributor

@christkv christkv commented Apr 7, 2013

Mongoose is a bit slow in comparison with the driver (about 1/2 of the performance) so changed it to use the mongodb driver directly

@bhauer
Copy link
Contributor

bhauer commented Apr 8, 2013

Hi @christkv. Thanks for the contribution! Just a heads-up that when we accept this pull request, we'll be converting it to become a new test that we'll name "nodejs-mongodb-raw." We aim for every test that does not include the "raw" suffix to use an ORM of some flavor.

We have some other tests that do not use an ORM, but rather their platform's native connectivity, such as "php-raw" and "servlet-raw." This additional raw test will be a great comparison data point.

@christkv
Copy link
Contributor Author

christkv commented Apr 8, 2013

sounds good :)

On Apr 8, 2013, at 3:57 PM, bhauer notifications@github.com wrote:

Hi @christkv. Thanks for the contribution! Just a heads-up that when we accept this pull request, we'll be converting it to become a new test that we'll name "nodejs-mongodb-raw." We aim for every test that does not include the "raw" suffix to use an ORM of some flavor.

We have some other tests that do not use an ORM, but rather their platform's native connectivity, such as "php-raw" and "servlet-raw." This additional raw test will be a great comparison data point.


Reply to this email directly or view it on GitHub.

@TechEmpower TechEmpower merged commit 2534aeb into TechEmpower:master Apr 8, 2013
@pfalls1
Copy link
Contributor

pfalls1 commented Apr 8, 2013

@christkv Thanks! Like @bhauer mentioned, I kept the original mongoose test, and now your test will show up as nodejs-mongodb-raw on our results.

@bhauer
Copy link
Contributor

bhauer commented Apr 9, 2013

Hi again @christkv. Would you and @bjornstar mind taking a look at issue #33 raised by @sidorares? He points out that since the queries are intended to be independent, we should either:

  • Remove the async and simply run the statements sequentially.
  • Use a separate connection for each statement that we are presently running using async.

Running async statements on the same connection yields a sequential run of the statements anyway because each connection can process only one statement at a time internally.

Note: In the JVM space, at high client concurrency within our tests (128 or 256), the value of server-side concurrency fan-out vanishes. We're already saturating the cores of the server's CPU at that level of client-side concurrency. Nevertheless, as I understand it, it is idiomatic in node.js to use async for scenarios such as running a bunch of independent queries as we are. I don't have a preference between the two above options; I would rather defer to folks with more node.js expertise.

@bjornstar
Copy link
Contributor

I don't see what's wrong with using async as a control flow library to collect the results of all the database queries. All of these tests are using connection pooling so it should be as parallel as it can get.

Also thanks @christkv for doing the raw version 😸

@sidorares
Copy link
Contributor

@bjornstar - no, it's not parallel, at least in nodejs-mysql-raw example. Each request gets connection from pool (which is ok) and then performs N queries on that connection sequentially. They are not run in parallel despite async.parallel(queryFunctions) - it's a limitation of mysql protocol and mysql client internally queues requests. To actually have them in parallel queryFunction have to acquire connection from pool on each call.

@bjornstar
Copy link
Contributor

I see what you're getting at. You want to have a connection for each request. Go for it. I don't think async is getting in the way of that.

@sidorares
Copy link
Contributor

See #125

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

6 participants