@Saahmed I tend to agree with what @patrickbajao said above. We all have different level of understanding even after several years of working within Code Review context. I think it's best to ask for a second opinion if you have any concerns or unsure of something.

When I proposed making this a required review, I wanted us to have a visibility of any changes within our domain as it often get updated by other team members.

I think @patrickbajao's list covers most of it, but here are some additional things I look for in Merge Requests backend

• Any new columns/indexes to already wide tables like merge_requets
• Encourage separation of concerns as already large models like MergeRequest can collect code that are better suited in other domain model.

Thanks @marc_shaw I left a few more comments

Right, IMO this would be us guessing what the intention of the user is and changing this would be a change in behaviour of this feature.

Yeah, I guess that depends on if we consider retargetting branch should only happen when the branch gets deleted. Let's leave this as is 👍

Hmm - I think it would be covered by the merge lease. Maybe we could do it before we let go of the merge lease?

I think merge lease only cover the aspect of merging the same MR, but I think ideally we'd like to prevent the child MR to be start merging into the parent MR when the parent MR is being merged currently. Obtaining lease just before queuing branch deletion worker seems a little too late as the child MR's merge might have already started if they happen in quick succession.

However, that also doesn't completely cover the gap. If a child MR gets queued to be merged first, but somehow takes longer to merge than the parent MR. I don't think this lease approach will cover that. I guess likelihood of that happening is pretty low though. 🤔

I think the way to get around that is obtaining two leases to prevent the merge altogether if any of its related MRs are currently being merged.

  return unless branch_lease(@merge_request.source_project_id, @merge_request.source_branch).try_obtain
  return unless branch_lease(@merge_request.target_project_id, @merge_request.target_branch).try_obtain

However, it means that we're sychronizing merges across all MRs as we need to obtain lease on the same target_branch in the case of non-chained MRs and that won't perform well. I suppose it's not worth it to cover that unlikely scenario, but we should obtain the lease at the beginning of the merge? WDYT?

Can we rename it to release_branch_deletion_lease or something? calling it a flag seems a little confusing.

Same as above, can we move this into let as well since we're repeating this several times?

Should we move this to a let so that we don't have to repeat this several time in the spec? We could then just call branch_deletion_lease.try_obtain or branch_deletion_lease.exists?

Can we make it a constant as well? Also, considering merge has 15 minutes lease timeout, should we match that?

It seems that difference between perform_with_race_condition_prevention and perform_without_race_condition_prevention is just the lease removal. Could we do just do something like this to simplify without duplicating the logic?

    remove_pending_deletion_flag(merge_request) if Feature.enabled?(:prevent_merge_race_condition, merge_request.project)

@ptalbert Those failing pipelines seem to be passing now, but we still have one more undercoverage issue. Can you resolve that please? Hopefully that's the last one 😅

@ptalbert Yeah, that seems unrelated to me as well.

It seems that !226304 (diffs) introduced that issue and !219392 (diffs) subsequently addressed that.

I'll kick off the pipeline again and hopefully it passes now.

Thanks @marc_shaw I've left one comment regarding a TODO, but not a blocker. Feel free to move on to the maintainer review.

Summary

Adds the new reviewer_assignment_strategy column to project_settings and renames all model constants, enums, attributes, and methods from code_owner_reviewer_* to reviewer_*. This prepares the reviewer assignment infrastructure to be reused by DAP (Duo Agent Platform) and other approval types beyond CODEOWNERS.

The old code_owner_reviewer_assignment_strategy column is ignored via ignore_column and will be dropped in a follow-up MR.

What does this MR do?

• Migration: Adds project_settings.reviewer_assignment_strategy column
• Model: Renames constant CODE_OWNER_REVIEWER_ASSIGNMENT_STRATEGIES to REVIEWER_ASSIGNMENT_STRATEGIES, updates the enum and attribute declarations
• Method: Renames code_owner_reviewer_auto_assignment_enabled? to reviewer_auto_assignment_enabled?
• Ignore column: Adds ignore_column :code_owner_reviewer_assignment_strategy for safe deployment
• Specs/config: Updates all references in specs and API attributes YAML

Why?

The DAP epic (&20711 - "DAP-Powered Intelligent Reviewer Assignment") depends on the assignment infrastructure from Epic &20708 ("Native CODEOWNERS Reviewer Auto-Assignment"). DAP will add its own strategy (e.g., duo_intelligent) to select optimal individual reviewers from approval groups. By making the column/enum generic now, we avoid a breaking rename later.

Related

• Epic: &20708
• DAP Epic: &20711
• Part 2 (drop old column): !227378
• Previous combined MR: !227050

Shouldn't we keep the TODO here just so we remember to clean it up if it was needed for that particular reason?

@marc_shaw Seems reasonable to me, but I think we need to rename it in multi stages. https://docs.gitlab.com/development/database/avoiding_downtime_in_migrations/#renaming-columns

Also, as discussed, I've disabled the FF for now. https://gitlab.slack.com/archives/C101F3796/p1773384869468289

Thanks @reprazent I've pushed up the change, but I had to just target auth check only as the mergeability check has update calls that cannot easily be avoided.

Also, I just left the preload in the worker as it seems quite specific to this purpose.

This is only called once in this context.