Skip to content
Snippets Groups Projects

Fix N+1 queries in Jira Development Panel API endpoint

Merged Sean McGivern requested to merge fix-n-plus-one-in-jira-github-api into master
All threads resolved!

What does this MR do?

There were three sources of N+1 queries here: the license check, the path and namespace information, and the root namespace.

The license check was the worst. We were checking the license information on each project individually. This meant we couldn't paginate in SQL, but did so in Ruby, so it was not only an N+1, it was loading too many records.

To fix this, we use the fact that this API endpoint can only return projects in a particular namespace. License checks end up at one of two places: for most instances, it's the instance's license itself. For GitLab.com, where individual namespaces have their own plan, it's the root namespace (subgroups can't have plans; they inherit their plan from the root).

This means that we only ever need a single check. If it passes, every project returned has the feature available. If it fails, we return a 404, like the other endpoints here. That way we can paginate in SQL, as we should.

The path and namespace information N+1 was simple to fix: just preload that information.

The final N+1 was on the root namespace, which we return as the owner field for compatibility with GitHub. Again, this was always the same for all items in the response, but we can't preload it easily because different projects will be at different levels of the hierarchy. Instead, we just calculate the root namespace once, and pass that as an option to the entity. The entity uses that value if it's given, and falls back to calculating it if it's not (in case this entity is used elsewhere without that option).

For #29741 (closed).

Screenshots

Does this MR meet the acceptance criteria?

Conformity

Performance and Testing

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Author Contributor

    @engwan as you have the context here, and you're a trainee maintainer, could you review this?

  • Sean McGivern assigned to @engwan and unassigned @smcgivern

    assigned to @engwan and unassigned @smcgivern

  • 1 Warning
    :warning: 6625a6d3: This commit’s subject line is acceptable, but please try to reduce it to 50 characters.

    Reviewer roulette

    Changes that require review have been detected! A merge request is normally reviewed by both a reviewer and a maintainer in its primary category (e.g. frontend or backend), and by a maintainer in all other categories.

    To spread load more evenly across eligible reviewers, Danger has randomly picked a candidate for each review slot. Feel free to override this selection if you think someone else would be better-suited, or the chosen person is unavailable.

    Once you've decided who will review this merge request, mention them as you normally would! Danger does not (yet?) automatically notify them for you.

    Category Reviewer Maintainer
    backend Alper Akgun (@a_akgun) Rémy Coutable (@rymai)

    Generated by :no_entry_sign: Danger

    Edited by 🤖 GitLab Bot 🤖
  • Heinrich Lee Yu
  • Heinrich Lee Yu
    • Resolved by Heinrich Lee Yu

      Thanks @smcgivern, looks good!

      Do you think we need to add a spec for the unlicensed case for this endpoint? I see that we don't have existing specs for this and we changed the behavior a bit here by now returning a 404 instead of an empty list.

  • Heinrich Lee Yu assigned to @smcgivern and unassigned @engwan

    assigned to @smcgivern and unassigned @engwan

  • mentioned in issue #33741 (closed)

  • Author Contributor

    Thanks @engwan! Could you take another look please?

  • Sean McGivern assigned to @engwan and unassigned @smcgivern

    assigned to @engwan and unassigned @smcgivern

  • Sean McGivern added 1 commit

    added 1 commit

    • a6ec2bc4 - Fix N+1 queries in Jira Development Panel API endpoint

    Compare with previous version

  • Heinrich Lee Yu approved this merge request

    approved this merge request

  • Thanks! LGTM :rocket:

  • Heinrich Lee Yu assigned to @smcgivern and unassigned @engwan

    assigned to @smcgivern and unassigned @engwan

  • Author Contributor

    Thanks @engwan!

    @tkuah could you review, please?

  • Sean McGivern assigned to @tkuah and unassigned @smcgivern

    assigned to @tkuah and unassigned @smcgivern

  • Thanks for the quick response on this @smcgivern :thumbsup: :heart:

  • Thong Kuah resolved all threads

    resolved all threads

  • Thong Kuah
  • Thong Kuah assigned to @smcgivern and unassigned @tkuah

    assigned to @smcgivern and unassigned @tkuah

  • Sean McGivern added 1 commit

    added 1 commit

    • 6625a6d3 - Fix N+1 queries in Jira Development Panel API endpoint

    Compare with previous version

  • Sean McGivern assigned to @tkuah and unassigned @smcgivern

    assigned to @tkuah and unassigned @smcgivern

  • Thong Kuah resolved all threads

    resolved all threads

  • Thong Kuah approved this merge request

    approved this merge request

  • merged

  • Thong Kuah mentioned in commit 71568a69

    mentioned in commit 71568a69

  • Thanks @smcgivern @engwan LGTM :rocket:

  • This merge request has been deployed to the GitLab.com environment gstg in GitLab auto-deploy version 12.4.201910171510-ac38081b602.8a20b2fd697.

    A list of all the deployed commits can be found here.


    :robot: If this message is incorrect, please create an issue in the Release Tools project.

  • This merge request has been deployed to the GitLab.com environment gprd-cny in GitLab auto-deploy version 12.4.201910171510-ac38081b602.8a20b2fd697.

    A list of all the deployed commits can be found here.


    :robot: If this message is incorrect, please create an issue in the Release Tools project.

  • added workflowcanary label and removed workflowstaging label

  • This merge request has been deployed to the GitLab.com environment gprd in GitLab auto-deploy version 12.4.201910171510-ac38081b602.8a20b2fd697.

    A list of all the deployed commits can be found here.


    :robot: If this message is incorrect, please create an issue in the Release Tools project.

  • added workflowproduction label and removed workflowcanary label

  • mentioned in issue #37012 (closed)

  • mentioned in issue #195432 (closed)

  • mentioned in issue #196862 (closed)

  • Please register or sign in to reply
    Loading