Add MergeRequestReviewer for dedicated Reviewers section

What does this MR do?

Related to #216054 (closed)

This is the first step towards adding dedicated reviewer section to MergeRequests. It'll be available for EE and I'll add a feature flag in controller level so I don't think we need documentation update at this stage.

FYI, this is the next WIP MR that uses this model. !40378 (closed)

Screenshots

Does this MR meet the acceptance criteria?

Conformity

• Changelog entry
• [-] Documentation (if required)
• Code review guidelines
• Merge request performance guidelines
• Style guides
• Database guides
• Separation of EE specific content

Availability and Testing

• Review and add/update tests for this feature/bug. Consider all test levels. See the Test Planning Process.
• Tested in all supported browsers
• Informed Infrastructure department of a default or new setting change, if applicable per definition of done

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

changed milestone to %13.4

added backend devopscreate groupsource code merge requests labels

changed title from Add merge_request_reviewers table to Add MergeRequestReviewer for dedicated Reviewers section

added database databasereview pending labels

	1 Warning
	5c2028bd: Commits that change 30 or more lines across at least 3 files must describe these changes in the commit body. For more information, take a look at our Commit message guidelines.

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 picked a candidate for each review slot, based on their timezone. Feel free to override these selections if you think someone else would be better-suited, or the chosen person is unavailable.

To read more on how to use the reviewer roulette, please take a look at the Engineering workflow and code review guidelines. Please consider assigning a reviewer or maintainer who is a domain expert in the area of the merge request.

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

Category	Reviewer	Maintainer
backend	Dallas Reedy (@dreedy) (UTC-7, 16.5 hours behind @dskim_gitlab)	Mark Chao (@lulalala) (UTC+8, 1.5 hours behind @dskim_gitlab)
database	Alper Akgun (@a_akgun) (UTC+3, 6.5 hours behind @dskim_gitlab)	Tiger Watson (@tigerwnz) (UTC+10, 0.5 hours ahead of @dskim_gitlab)

If needed, you can retry the danger-review job that generated this comment.

Generated by Danger

removed database databasereview pending labels

added Enterprise Edition typefeature labels

changed the description

added 1 commit

• ce3e7c06 - Add merge_request_reviewers table and model

Compare with previous version

added database databasereview pending labels

added 1 commit

• f3029810 - Add merge_request_reviewers table and model

Compare with previous version

added 1 commit

• 793c97dd - Add merge_request_reviewers table and model

Compare with previous version

mentioned in merge request !40378 (closed)

@matteeyah Would you be able to do backend review please

@nmilojevic1 Would you be able to do database review please?

assigned to @matteeyah and @nmilojevic1

changed the description

added 1 commit

• 793c97dd - Add merge_request_reviewers table and model

Compare with previous version

approved this merge request

Nice work @dskim_gitlab. This basically just adds a join table, right? LGTM

unassigned @matteeyah

assigned to @ayufan

mentioned in merge request !40424 (merged)

mentioned in merge request !40488 (merged)

unassigned @ayufan

Can you please check our guidelines for adding a new table with two foreign keys: https://docs.gitlab.com/ee/development/migration_style_guide.html#examples

For creating a new table when we have two foreign keys:

It is suggested to have three migrations:

• Creating the table without foreign keys (with the indices).
• Add a foreign key to the first table.
• Add a foreign key to the second table.

Also, create_table is suggested to be used with with_lock_retries, and with_lock_retries method cannot be used within the change method, you must manually define the up and down methods to make the migration reversible. https://docs.gitlab.com/ee/development/migration_style_guide.html#when-to-use-the-helper-method

With 2 foreign keys to be created in the same transaction, we first acquire a lock on the first target table. After creating the FK, we hold on to that lock and try to acquire the lock on the second table.

So with 2 FKs in the same transaction, we risk locking the first target table for an extended period of time.

changed this line in version 6 of the diff

Thanks a lot for your suggestions @nmilojevic1.

I missed the database guideline!

rubocop was complaining that we should use add_concurrent_foreign_key instead, but I felt using with_lock_entries seems more relevant so I disabled the cop. Please let me know if I missed anything.

Also, I don't think with_lock_entries is required for creating a new table? Is there any reasoning behind that? I couldn't find anything on the doc about that either.

rubocop was complaining that we should use add_concurrent_foreign_key instead

There is no need to use add_concurrent_foreign_key on a new table with no data.

Rubocop is complaining because it can not figure out that this is a new table (it was created in another migration/file), so it defaults to complaining even if there are some false positives like this case.

I think it's ok to disable the cop.

Also, I don't think with_lock_entries is required for creating a new table?

Yes, now, with separated migrations for adding new foreign keys, there is no need for with_lock_entries.

Well, this part is maybe a little confusing in the documentation.

Creating a new table with a single foreign key, it is ok to add a reference column when creating a table in the single transaction, but we should simply wrap the create_table method with_lock_retries because it will also lock the targeted table which is not empty.

When we add several foreign keys, we risk locking the table for a long time and hat's probably the main reason why we suggest splitting this up if there are multiple foreign keys involved.

with_lock_retries guards only against too long lock acquires, but it doesn't guard you when you are already holding other locks in the transaction.

This way, when we have separate migration for each foreign_key wrapped in a with_lock_retries, it guarantees that we provide each DDL operation the best possible conditions to succeed without competing for time.

Please check @iroussos's very good and detailed explanation here for more details.

Thanks, @dskim_gitlab. database LGTM

@tigerwnz can you please do the maintainer review for database?

LGTM thanks @dskim_gitlab!

Thanks a lot for your detailed explanation and a link

It makes a lot of sense and I agree it'd be great if we can update the doc to clarify the reasoning as well.

Thanks a lot for your review as well @tigerwnz

assigned to @ayufan and unassigned @nmilojevic1

unassigned @ayufan

added 1 commit

• 5c2028bd - Split migration into three and replace validation with unique index

Compare with previous version

assigned to @ayufan

assigned to @nmilojevic1

changed the description

approved this merge request

unassigned @ayufan

@dskim_gitlab LGTM. I believe we still have database review pending.

added 739 commits

• 5c2028bd...03857a62 - 738 commits from branch master
• 8aef55d9 - Merge branch 'master' into add-reviewers-to-merge-requests

Compare with previous version

added databasereviewed label and removed databasereview pending label

assigned to @tigerwnz

unassigned @nmilojevic1

approved this merge request

added databaseapproved label and removed databasereviewed label

resolved all threads

enabled an automatic merge when the pipeline for 0aa521a9 succeeds

merged

mentioned in commit c09f0508

added workflowstaging label

added workflowcanary label and removed workflowstaging label

added workflowproduction label and removed workflowcanary label

This merge request has been deployed to the pre.gitlab.com environment, and will be included in the upcoming self-managed GitLab 13.5.0 release.

---

This comment is generated automatically using the Release Tools project.

added published label

This merge request has been deployed to the release.gitlab.net environment, and will be included in the upcoming self-managed GitLab 13.5.0 release.

---

This comment is generated automatically using the Release Tools project.

added Category:Code Review Workflow groupcode review labels and removed groupsource code label