Handling Code Ownership

The title is very broad, but I honestly cannot come up with a decent question, so please bear with me.

Recently we have introduced a code ownership automation via Packages. Whenever someone posts a revision, Phabricator will summon several teams as reviewers, depending on which files are affected. As I have email notifications turned on, I will get emails saying that someone posted a revision I should look at. So far so good.

We have several MLOC codebase mostly in C++ that is unfortunately rather tightly coupled, so it is frequent that someone changes things in the area of responsibility of several teams at once. Thus we introduced a policy that a team should be strictly responsible for the files in their area, but is free skip the rest of revision. E.g., the revision changes code about video analytics and code about video archive storage – the archive guys should thoroughly check the archive stuff, but they might very likely get lost in the analytics stuff, so they are free to not review. And vice versa

My team specifically is responsible for automation tests, Jenkins integration, CMake build ecosystem etc. and in our case we can simply drown in the amount of notification flooding us. Example: someone posts a revision adding new source files to be built in CMakeLists build script, adding new Jenkins test, and changing 10 more files adding some business logic. We review the build and test changes – and they are basically set in stone for the rest of revision. Well, maybe there will be more files to be built added to the list of files.
However other people notice some flaws in the business logic code itself, so the revision goes into 20 more “declined”, “updated”, “accepted”, “changes planned”, “review requested” iterations. All the while the stuff we reviewed stays unchanged, but we recieve a ton of email notifications about changes and discussions we are not even interested in. It honestly drives me nuts and makes me want to turn off notifications altogether – which would hinder my reactions to people reviewing my revisions and replying to my comments in theirs.

So… can we do something to make our life as reviewers easier? Is there something fundamentally wrong with our approach, since I notice that Phabricator sometimes relies on some unusual (for us) workflows? I think simply having more options to filter notifications in Email settings would help buuuut I think it would require a lot more options to really make difference.

The problem in your workflow is that you need a committee to approve each change; Your team gets hit the worse because all changes go through you, but all other teams have the same issue, to a lesser extent.
In addition to the noise, you also risk having stuff fall through the cracks - you’re done with reviewing the tests, but then the business logic changes, and the tests are being updated, but the update is not being reviewed again because you’ve already approved the revision.

Here are a some ideas on how to change your workflow to reduce noise:

  • Split these changes to multiple revisions - one for the tests, one for the archive stuff, one for analytics, etc. Combine these revisions using depends on relation.
    This way you’ll only need to review your part of the change, and you will not be notified about the rest of the stuff.
  • Don’t monitor CMakeLists and tests. Have each team own their tests; You own the frameworks for testing.
    This way when they add a test, you don’t need to review it. You’ll want to add lints/tests to make sure nobody is doing something really strange in your CMakeLists, etc.
  • Unsubscribe from the revision after the initial approval. Maybe unsubscribe from the Package/Project as well.
  • Don’t review these changes until everyone else is done. Ignore emails. Wait for the author to ask you directly to review it.
  • Have a design review step for each change; Get the business rules well-defined before code gets written/distributed to all teams.
    Once the design review is done, add the code for all the pieces and create the real Revision.
  • Work on reducing coupling between teams’ code.

Alternatively, you can try to make some changes to the way Phabricator/emails works:

  • Use Herald rules for email notifications to disable emails about revisions that don’t have the words “AUTO TEST TEAM PLS CHK” in the body (or some custom field). Manually add/remove these words as applicable.
  • Disable emails about revisions, since you’re not using it anyway, and only use the Must Review section in the dashboard. Maybe unsubscribe from the relevant Projects.
  • Make a filter in your email client to ignore emails about Revisions unless it’s “Revision Updated” and the list of files include “CMakeLists” (or maybe your name is mentioned).

Those are all valid points, thank you for your reply.

Regarding unsubscribing from revisions: is it possible to unsubscribe from the revision that you were assigned to by Package? I noticed couple of times that the “unsubscribe” button was unavailable; I could “Resign as reviewer”, but I feel this is kinda not the same.

Regarding Herald - can it be used to prevent email notifications, like “if the rule is matching, do not send notification”, without turning off the whole notification system? We are planning on patching Arcanist so it would post “Changes planned” state as part of the main diff update, not a separate transaction, and I think I could come up with some heraldics to at least prevent the spam on revisions that are still WIP.

I also presume there is no option to “unsubscribe from revision until files from your Ownership are changed again”. Is there a chance it will appear at some point?

Regarding unsubscribing from revisions: is it possible to unsubscribe from the revision that you were assigned to by Package? I noticed couple of times that the “unsubscribe” button was unavailable; I could “Resign as reviewer”, but I feel this is kinda not the same.

You may be able to unsubscribe from the Package rather then the revision.

Regarding Herald - can it be used to prevent email notifications

Should be able to, yes, but I didn’t try this specific scenario.

…revisions that are still WIP

There’s currently(?) a --plan-changes flag for Arcanist, but it’s being deprecated in favor of Draft Mode. See https://secure.phabricator.com/T2543 and https://secure.phabricator.com/T13010 for Draft Mode - I think it mostly works now.

"unsubscribe from revision until files from your Ownership are changed again”

I don’t think that’s really doable; A big issue here is that we don’t actually represent “change in a revision” as a “thing” you can monitor - Herald looks at Revision current state, and it’s being triggered when a revision is modified, but the transactions themselves are ignored. So we don’t actually know if some file in this state of the revision is different from the previous state.

The Draft Mode looks promising, but it seems like it is still WIP. How exactly can we turn it on?

Also, T13010 mentions

Once builds pass, they are submitted for review. (If you don’t use Harbormaster to perform builds, all builds automatically pass immediately, so this state is transient.)

Is there a way to keep revision in draft until the reviewer themselves say it is OK to review, like we currently do with --plan-changes?

I’m not sure, you’ll have to play around with it and see.