Skip to content

Explain pros/cons of vertical/horizontal slicing

Dylan Griffith requested to merge vertical-rather-than-horizontal-mr-slicing into master

For additional context, some previous content that's being discussed here was added in !47946 (merged) .

Major update to this MR

The original version of this change expressed a strong opinion that we should avoid horizontal merge requests and always prefer vertical slices. Mainly linking up to the MVC concept. While getting feedback on the tradeoffs between vertical and horizontal slicing (most of the comments on this MR as of 2020-09-24) I learnt that some teams and some maintainers felt they could only be productive if the MRs are sliced into horizontal slices. Further I learnt that frontend maintainers particularly felt that the frontend code often needed to be sliced further as well into layers of the frontend stack. So rather than discouraging horizontal slicing with this change I'm highlighting the challenges of it as well as how to help improve the context you provide to the reviewer.

This doesn't solve all my concerns about the chance that code needs to be rewritten when it is later integrated and it does not solve the specific example in which I raised a performance concern in a partially complete MR that later ended up having the same problem I warned about. But it does at least help document what many people are already doing to ensure the full context is provided to the reviewer.

Why is this change being made?

There has recently been some discussion at gitlab-org/gitlab!37782 (comment 388186156) and https://gitlab.slack.com/archives/CJHPLRTRD/p1596516098443100 which indicates there is some discrepancy in our approach to splitting MRs at GitLab as well as the preferences of Maintainers reviewing and merging these MRs.

Primarily there appears to be a few maintainers that have expressed a preference for encouraging MRs to contain the complete scope of a change so that they can give the best possible feedback on scalability, architecture and general usability of a feature.

As a maintainer myself I've found that roughly half the MRs I review are partial steps towards a feature that do not, on their own, provide immediate value. After being a GitLab user for several years and a developer on several parts of the GitLab application I believe the best opportunity to provide meaningful feedback during code review is when I see a complete picture. I may even opt to test it locally. But when I'm given an MR which just includes a new backend Service class with nothing consuming it I find it really hard to give the most valuable feedback and sometimes when I do pre-emptively give certain feedback I get the suggestion that such feedback will be accommodated in the final product but they aren't always. I believe that MRs that change code out of context often lead to me feeling like my role as a maintainer is just to catch superficial things like code style etc. because I can't actually see enough of the picture to provide any better insights.

Specific examples

I had 2 recent examples I wanted to point out where I believe the approach to separating MRs has led to less than ideal outcomes. I don't want to call out the specific developers or reviewers involved in these cases and my assumption is that our current guidelines splitting MRs are leading, inevitably to this outcome, which seems to be a similar experience other maintainers are noticing.

(1) Feedback on a partial implementation which was agreed to but later missed which can lead to very poor performance

A while back I reviewed an MR that was only a small part of the necessary backend code. When giving this feedback I noticed some code that concerned me from a performance perspective but I couldn't be sure it was going to cause performance issues until I saw how it was actually invoked. I did leave a comment to this affect to make sure to invoke this code only in sidekiq/background processes but I never was involved in reviewing any of the later MRs for this feature and it subsequently shipped with the exact scalability concern I noted.

(2) An MR for backend that received several rounds of feedback and was almost entirely replaced in a subsequent MR

Recently I reviewed an MR that introduced a backend API that served up entirely static data from a JSON file gitlab-org/gitlab!35397 (merged). This at the time seemed like a good approach to cutting scope to keep the development moving. In this case, however, the new API was separated from any frontend implementation and as such I couldn't raise any concerns I might have about the edge cases to user experience or whether or not a different engineering approach could be taken. While the initial MR still generated 57 discussion items and involved quite a few back and forth reviews from several people it seems (from what I can tell and I may be missing details) that the backend code shipped for that API change are almost completely being rewritten in gitlab-org/gitlab!36989 (merged) still before there is any frontend consumer of this. And even that MR is quite large and generating quite a bit of architectural feedback which we maybe should have been dealing with earlier since it seemed this was always necessary before the feature could be viable.

Author Checklist

  • Provided a concise title for the MR
  • Added a description to this MR explaining the reasons for the proposed change, per [say-why-not-just-what][transparency]
  • Assign this change to the correct DRI
    • If the DRI for the page/s being updated isn’t immediately clear, then assign it to your manager.
    • If your manager does not have merge rights, please ask someone to merge it AFTER it has been approved by your manager in [#mr-buddies][mr-buddies-slack].
    • If the changes relate to any part of the project other than updates to content and/or data files please make sure to ping @gl-static-site-editor in a comment for a review and merge. For example changes to .gitlab-ci.yml, JavaScript/CSS/Ruby code or the layout files.
Edited by Dylan Griffith

Merge request reports