On Thu, Jul 13, 2017 at 6:04 PM, Mark Côté <mc...@mozilla.com> wrote:

> On 2017-07-13 3:54 PM, Randell Jesup wrote:
>
>> On Wed, Jul 12, 2017 at 11:27 AM, Byron Jones <g...@mozilla.com> wrote:
>>>
>>> But indeed having also the patches in bugzilla would be good.
>>>>
>>>>>
>>>>> no, it would be bad for patches to be duplicated into bugzilla.  we're
>>>> moving from bugzilla/mozreview to phabricator for code review,
>>>> duplicating
>>>> phabricate reviews back into the old systems seems like a step backwards
>>>> (and i'm not even sure what the value is of doing so).
>>>>
>>>
>>> I find this a strange argument.  We don't know how successful
>>> phrabricator
>>> is going to be.  The last attempt to switch review tools seems to be
>>> getting killed.  I think its somewhat reasonable to be skeptical of
>>> further
>>> review tool changes.
>>>
>>
>> I quite agree...  And I hate the idea of having the data spread across
>> systems (which I also disliked in MozReview, which I tried and it ended
>> up being really bad for some aspects of my ability to land patches).
>>
>
> Mirroring comments from Phabricator to BMO is even more confusing, as we
> end up with forked discussions, where some people reply only on BMO. This
> happens regularly with MozReview, as a quick survey of recently fixed bugs
> shows.  Keeping code review in one place, loosely coupled to issue
> tracking, improves readability of bugs by keeping discussion of issues
> separate from specific solutions.  It is also what newer systems do today
> (e.g. GitHub and the full Phabricator suite), which I mention not as a
> reason in and of itself but to note precedent.  The plan is to move to a
> separate, more modern, and more powerful code-review tool. Forked
> discussions means that the bugs will always have to be consulted for code
> reviews to progress, which undermines the utility of the code-review tool
> itself.  Loose coupling also frees us to try experiments like patches
> without bugs, which has been discussed many times but has been technically
> blocked from implementation.
>

> That said, lightweight linkages between issues and code review are very
> useful.  We're doing some of that, and we're open to iterating more there.
>
> We've been asked to be bold and innovate all across Mozilla, to try new
> things and see if they work.  For a long time, Mozillians have discussed
> modernized workflows, with new tools that also open more avenues to
> automation, but such things have been rarely attempted, and even then in a
> disorganized fashion.  We're finally at a time when we have the ability and
> support to forge ahead, and that's what we're doing.
>


This.

As someone who worked on MozReview, I can say unequivocally that one of the
hardest parts - and I dare say the thing that held MozReview back the most
- was the tight coupling with Bugzilla and the confusion and complexity
that arose from it.

When we deployed MozReview, we intentionally tried to not rock the boat too
hard: we wanted it to be an extension of the Bugzilla-centric workflow that
everyone was familiar with. Was there some boat rocking, yes: that's the
nature of change. But we went out of our way to accommodate familiar
workflows and tried to find the right balance between old and new. This
resulted in obvious awkwardness, like trying to map ReviewBoard's "Ship It"
field to Bugzilla's complicated review flag mechanism. Does a review that
doesn't grant "Ship It" clear the r? flag or leave it? What happens when
someone grants "Ship It" but they don't have permissions to leave review
flags in Bugzilla? How do you convey r-? What about feedback flags? What
happens when someone changes a review flag in Bugzilla? Then you have other
awkwardness, like when you mirror the review comment to Bugzilla and it
loses rich text formatting. Or someone replies to a MozReview comment on
Bugzilla and you can't see that reply on MozReview. Do you disable replying
to comments mirrored from MozReview? That's annoying. Do you disable the
mirroring then? That's annoying too! Bi-directional sync? No way! (If
you've ever implemented bi-directional sync you'll know why.) You just
can't win.

The usability issues stemming from trying to overlay ReviewBoard's and
Bugzilla's models of how the world worked were obvious and frustrating. It
took months to years to iron things out. And we arguably never got it right.

Then there were technical issues. When you did things like post a series of
commits to review on MozReview, this was done so as an atomic operation via
a database transaction. It either worked or it didn't. But we had to mirror
those reviews to Bugzilla. Bugzilla didn't have an API to atomically create
multiple attachments/reviews. So not only was the publishing to Bugzilla
slow because of multiple HTTP requests, but it was also non-atomic. When
Bugzilla randomly failed (all HTTP requests randomly fail: it is a property
of networks), the review series in Bugzilla was sometimes left in an
inconsistent state because it wasn't obvious how to recover (depending on
where the error occurred). So in addition to mapping alien concepts between
two systems, we're trying to implement distributed transactions. It was a
nightmare. And don't get me started about the technical complexity to
perform automated integration tests between these systems and the pain and
suffering we inflicted on MozReview developers forcing them to use fragile
Docker environments so we could accurately test Bugzilla interop. It
drastically slowed down development and made MozReview not pleasant to hack
on.

The cost to tightly couple MozReview with Bugzilla was significant. I argue
it was its fatal flaw. Instead of spending time integrating automatic code
linting, merge conflict notification, file watching (a feature built in to
Phabricator that I think many will love), automatic reviewer selection,
bug-less reviews, better Try and Autoland integration, a cleaner UI,
ingestion of GitHub pull requests, etc, we spent time trying to hammer a
square peg into a round hole. Instead of adding extra value via new
features that weren't possible with Bugzilla/Splinter, we spent our time
trying to make MozReview compatible with a dated, Bugzilla-centric workflow.

While I - like everyone - have concerns about Phabricator (it is rational
to question changes to high impact tools/workflows), I do support the
decision to decouple code review from Bugzilla. We tried tight coupling
with MozReview. And we *really* tried to make it work. But no matter what
we did, it felt awkward and was brittle and time-consuming to implement. If
you had the experience Mark, myself, and anyone who significantly worked on
MozReview had, you would see clear as day how so many problems stemmed from
the decision to tightly integrate the two tools. Then if you take a step
back, approach the problem from first principles, and survey the modern
tooling landscape, you realize how antiquated the workflow we were trying
to support actually is. It becomes clear that the best long-term outcome is
to unshackle code review from Bugzilla. It's a controversial decision and
will potentially be a disruptive change, yes. But I'm pretty confident that
if you had the experience that we did with MozReview and were empowered to
make difficult - but justifiable decisions - you too would reach the same
conclusion. I say this not as someone who was involved in the decision to
stop development on MozReview, switch to Phabricator, and turn off Splinter
(I wasn't really involved and was even surprised by parts of the plan like
everyone else): I say this as an engineer who worked on MozReview and want
what's best for Mozilla long term. Decoupling code review from Bugzilla is
a justifiable, forward-thinking, and overdue change. I support the decision
and applaud its ambition.
_______________________________________________
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Reply via email to