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

Resolve N+1 issue in GetDocuments API response when snapshot is included #933

Open
kokodak opened this issue Jul 20, 2024 · 1 comment
Open
Assignees
Labels
enhancement 🌟 New feature or request

Comments

@kokodak
Copy link
Member

kokodak commented Jul 20, 2024

What would you like to be added:

Currently, when the GetDocuments API endpoint is called and the response needs to include a snapshot, an N+1 issue occurs. This problem arises because packs.BuildDocumentForServerSeq() is called internally N times.

Within packs.BuildDocumentForServerSeq(), two database queries are executed, leading to the N+1 problem. To resolve this issue, the database query within this method should be converted to a bulk version.

If there is a need for a snapshot preview when displaying documents in CodePair, resolving this issue could result in performance improvements.

if includeSnapshot {
// TODO(hackerwins, kokodak): Resolve the N+1 problem.
doc, err := packs.BuildDocumentForServerSeq(ctx, be, docInfo, docInfo.ServerSeq)
if err != nil {
return nil, err
}
snapshot = doc.Marshal()
}

Why is this needed:

To address the N+1 problem in the GetDocuments API response to enhance performance.

@kokodak kokodak added the enhancement 🌟 New feature or request label Jul 20, 2024
@kokodak
Copy link
Member Author

kokodak commented Aug 2, 2024

kokodak@e600891

The attached commit is the code for a bulk query implementation I've worked on before.

In the case of FindClosestSnapshotInfo() bulk, the required query is rather complicated, so I configured the query by configuring the Aggregate pipeline.
When sorting and grouping in the pipeline, it would be good to consider the DB Index applied to the Snapshot Collection.

For the bulk of FindChangesBetweenServerSeqs(), there is a high probability of memory related issues, as it is looking for a very large number of Changes if a Snapshot is missing, and we need to be aware of this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement 🌟 New feature or request
Projects
Status: In progress
Development

No branches or pull requests

1 participant