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

TODOs #21

Open
1 task done
gr0uch opened this issue Jul 13, 2017 · 15 comments
Open
1 task done

TODOs #21

gr0uch opened this issue Jul 13, 2017 · 15 comments

Comments

@gr0uch
Copy link
Member

gr0uch commented Jul 13, 2017

Mostly new features:

  • Implement beginTransaction and endTransaction using the $isolated operator. It doesn't provide rollbacks but it's better than nothing. Using native implementation for MongoDB 4.0+
@ansarizafar
Copy link

MongoDB 4 now supports transactions. Does fortune mongodb support transactions and aggregations? Is it possible to get mongodb driver instance? How fortune find method works? Does it use multiple mongodb find commands or mongodb aggregation pipeline to get related documents?

@gr0uch
Copy link
Member Author

gr0uch commented Oct 10, 2018

Ah yes, as you can see this was written before MongoDB 4. I'll look into implementing transactions. It does not use the aggregation pipeline. It seems to be useful for optimizing include option though.

You can get a reference to the underlying client by using this.client:

this.client = client

@ansarizafar
Copy link

Mongodb aggregation is the new find, especially after the introduction of $lookup operator. Multiple find statements === multiple database roundtrips which means less performance.

@gr0uch
Copy link
Member Author

gr0uch commented Oct 10, 2018

@ansarizafar multiple calls to find are not made unless used in conjunction with include option, that is why I mentioned it. Calling find once === one database roundtrip.

@ansarizafar
Copy link

ansarizafar commented Oct 10, 2018

I am talking about related documents which at the moment means multiple finds and multiple database roundtrips. I actually wanted to use https://github.com/genie-team/graphql-genie with Sapper https://sapper.svelte.technology/ for my new project. Graphql-genie uses fortunejs for data access. Graphql already has N+1 request problem and with nested queries, it becomes a huge performance issue. I request you to consider using mongodb aggregation pipeline with $lookup operator for all related document (include) queries. Please also add support for data aggregation.

@gr0uch
Copy link
Member Author

gr0uch commented Oct 10, 2018

I'm not sure if the concern is warranted, I hope I'll try to explain it better. Fortune.js makes one roundtrip to the database (one call to find) per request, unless include is specified. Then it makes subsequent requests based on the include depth multiplied by number of included fields. For a typical use case of including 1-2 relationships, this would mean up to 3 roundtrips, independent of how many records are returned. The n+1 problem pertains to making separate database requests per record, which Fortune.js does not do, it actually batches all of the data at each nested level of include.

I agree that 1 aggregate request is faster than a few separate requests, I just don't think it would be that much faster since there is a fixed upper bound on the number of requests.

@gr0uch
Copy link
Member Author

gr0uch commented Oct 10, 2018

Transaction support has been added: 2d059cd

Published as 1.3.0.

Please note that this is an opt-in feature, since by default, MongoDB does not run in replica set mode, which is required for transactions.

@ansarizafar
Copy link

Thanks for adding transaction support. As mentioned above I am only talking about related records, multiple relationships mean multiple database roundtrips which is bad for a production-ready app/website with heavy traffic. MongoDB now has a better way of getting related documents with aggregation pipeline and $lookup operator is the recommended method

Aggregation is new find

I have read this quote somewhere on Mongodb website. My question is why multiple database round trips when we can fetch related documents with a single database request. I know graphql N+1 problem is a different issue but If fortunejs is used to fetch related records with a graphql https://github.com/genie-team/graphql-genie then it can become a huge performance bottleneck as graphql supports multi level nested queries. I would request you to consider using aggregation pipeline with $lookup operator to featch related records with one database request.

@gr0uch
Copy link
Member Author

gr0uch commented Oct 11, 2018

I don't think you read what I wrote. Even with nested includes, the number of requests is O(1) since the nesting level is constant. You might be mistaken in thinking that it makes a seperate call to find for each related record, which is actually not the case. Related records are batched into as few finds as possible (specifically, the depth of included records). There is no N+1 problem, so I don't think your concern is legitimate.

@ansarizafar
Copy link

I think I have used the wrong terminology. Fortunejs performs one database roundtrip for each related collection. Multiple related collections mean multiple round trips. Is that correct? With $lookup we need a single round trip.

@gr0uch
Copy link
Member Author

gr0uch commented Oct 11, 2018

I think there is no real problem, please let me know if you have actually analyzed database queries and found that there were excessive requests being made. I will wait.

In practical terms, you might have one or two nested includes for the vast majority of requests, which results in at most three round-trips, not an unbounded number. The complexity of adding such an optimization vastly outweighs the performance overhead of a constant number of additional queries. You seem to only understand that 3 is greater than 1, but you don't get that constant time is much better than linear time, so much that optimizing this will only see marginal improvements.

@ansarizafar
Copy link

Fortunejs approach was fine in old days but since Mongodb 3.6 $lookup is recommended by mongodb to fetch related collection. I was raising this issue in the context of Graphql specially https://github.com/genie-team/graphql-genie which is using fortunejs for data access. A multi-level nested graphql query with fortunejs means tens of database requests. Anyways If you don't like this idea then its ok.

@gr0uch
Copy link
Member Author

gr0uch commented Oct 11, 2018

Do you understand that the version of MongoDB is not relevant to the topic at hand?

Please specify what "tens of database requests" means. It isn't even worth the effort arguing about this.

@ansarizafar
Copy link

Mongodb version is relevant as Mongodb introduced this new recommended method of fetching the related collections with $lookup with version 3.6. Anyone who has used graphql can understand what I mean. As I said its ok If you don't want to implement this feature. It's your project and you have every right to decide what you want.

@gr0uch
Copy link
Member Author

gr0uch commented Oct 11, 2018

Whether one is using MongoDB less than 3.6 or greater than 3.6, the performance gains from using $lookup are minimal, and may even be slower! How is it possible that 2 requests are faster than 1?

Let's say that hypothetically, there are 10,000 records, each of them related to 1 record. By performing a $lookup on these 10,000 records, it will try to find this 1 record 10,000 times, and then it will embed this 1 record 10,000 times in the response. I am not even kidding, look at the examples in the official documentation for this pattern. Performance characteristics are very complex and can be hard to predict. You might have some theoretical concerns if the nesting level is unbounded, but in real world applications it is usually bounded.

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

No branches or pull requests

2 participants