Skip to content

WIP: Fill merge request discussions from paginated notes

Nick Thomas requested to merge 217673-keyset-paginate-notes into master

What does this MR do?

Per #217673 (closed) and #209784 (closed), the merge request discussions endpoint is slow for large MRs. It's slow because it's not paginated.

We want the backend to reliably return in under 500ms which, for discussions, unavoidably means gathering the content in multiple backend calls:

  • Noteables can have an unlimited number of discussions
  • Discussions can be composed of an unlimited number of notes
  • Notes can have up to 1MiB of user-specified markdown
  • That markdown is pre-processed into HTML, but must be reprocessed per-user for confidentiality

An additional challenge with the discussions endpoint is that we don't have a "real" Discussion model in the database - it's a virtual model. This makes paginating on it fairly hard.

A further additional challenge is that not all the notes we want to show are "real" Note instances either - we have to consult the resource_*_events tables, each in a slightly different way, to construct pseudo-notes in pseudo-discussions, if we're to get the full set of data currently presented in the discussions endpoint.

Doing all this efficiently, and in an understandable and non-fragile way, is quite difficult. This MR takes an alternative approach: paginate the notes endpoint.

We still have to worry about synthetic notes, but not the synthetic discussions that they're in, and since the notes are presented as a flat list, we can efficiently paginate it in a very simple way on the backend.

The frontend already polls this endpoint for "notes updated since time X". This MR starts that "time X" at 0, skips pulling anything from discussions.json, and leans on the pagination of the endpoint notes.json` endpoint. This makes us progressively build the content of the MR discussions tab up from nothing, requesting data a page at a time.

There's a lot of concern about the UX effects this may have. Once I've gotten this MR to a state I'm happy with, I plan to demo it so we can characterise the changes, and decide whether this is an acceptable state to be in or not.

Currently, it looks like there are no user-visible presentational changes - we display the loading skeleton as before, and only show the full list of notes once we've collected the full list from the backend. Larger MRs may take marginally longer to load, due to per-request overhead that wasn't there before, but on the other hand, the notes endpoint is less complicated than the discussions endpoint, so this may end up being faster in some cases, and is certain to succeed in cases where the old approach would reliably time out.

This is only applied to MR discussions at the moment but could be applied to any noteable in the future.

Screenshots

Peek_2020-06-03_17-15

Does this MR meet the acceptance criteria?

Conformity

Availability and Testing

Security

If this MR contains changes to processing or storing of credentials or tokens, authorization and authentication methods and other items described in the security review guidelines:

  • Label as security and @ mention @gitlab-com/gl-security/appsec
  • The MR includes necessary changes to maintain consistency between UI, API, email, or other methods
  • Security reports checked/validated by a reviewer from the AppSec team

Closes #217673 (closed)

Edited by Nick Thomas

Merge request reports