Vrftsjtryk

Either a code review is required for the release, or it is not required. If it is required, then the reviewer must be responsible for reviewing this in time. You must have the power to go to his desk and say "drop everything else and do this review, or we can't make the release date". And you must have the power to say "sorry, we couldn't release in time because the review wasn't done in time".

If it is not required, then you release.

PS. If Clint is given one day to review weeks or months of work, that seems to indicate that the review wasn’t needed. But if Clint finds problems in this short time that seems to indicate that previous reviews were not quite as good as they should have been.

It sounds like there are a number of issues here, but all of them are addressable.

Process issues:

• What is the purpose of this pull request? Is it meant to be reviewed, or is it simply a means to merge to master from a QA'ed and tested branch? If the former, then you should consider different scheduling for the review process. If the latter, it should be merged almost the instant it's created.


• What do you do with "late" review comments? It sounds like Clint's comments didn't go through the full review process, even though they were "late". (What defines "late"?) Even if you've committed to merging and none of the comments are show-stoppers, it should be possible to classify his comments as actionable or not, and reply in the PR as appropriate.

Personal issues:

• I'm getting a bit of a "we're not all one team" vibe here. This may stem from understandable frustration on your part, but if you respect Clint -- and I gather you do -- it's appropriate to try to fix this. He may be turning down your review meeting requests now because he's busy, or it may be that he feels that you really don't want his input anyway.


• Backfill to show your real intentions. If you think that some of the things he said were real issues, file tickets, and make sure that he's included on them.

Leaving the PR open for "real review" up till the evening before the push means that either you need to take all the reviews seriously and not merge and not push if there are unaddressed comments, or you need to change the push scheduling so that there's sufficient time to complete the review and to schedule or cancel the push (e.g., PRs not merged at least 24 hours before the scheduled push don't go out).

It might be a good idea to invite him for a coffee and talk it over, letting him know that yes, you do value his input, but that the process as it is right now meant that you'd not gotten his review early enough to act on it this time, emphasizing "this time". Let him know you're looking at changing the process, and ask him if he has any suggestions.

Ask him what would make it easier for him to contribute, and make sure he understands that you appreciate that he bothered. If he is really trying to help, then no action on his comments demotivates him too. It sounds like there were no stoppers, since you went ahead with the merge, but it also sounds like the work, from his point of view, was wasted effort.

Thank him for finding the issues he pointed out and let him know that you intend to address the significant things he brought up. You should consider doing that both one-to-one with him and publicly in the closed PR to make it clear to him and the team that you're glad he's helping. Showing gratitude, in private and in public, for his caring and volunteering to help, even if none of the review comments were actionable from your team's point of view, is just the polite thing to do, and it helps build solidarity between teams.

How can I tell Clint if he wants to provide feedback, it needs to be
on our terms?

Unless Clint works for you, or there is some sort of formal process which dictates when comments are not permitted, you can't force Clint to adhere to "your terms".

You can ignore his comments. You can thank him for the "somewhat helpful" comments to encourage more of those. You can add items to your technical backlog to address those helpful comments. You can continue to invite him to code review meetings. You can encourage your team not to get demotivated based on a few late comments.

1) Is there nobody between you and 20 engineers who can help you mentor them? This is why 20 people is too many people on a team, because there's no way one person can manage and mentor 20 people. You need to cut the size of your team, or at least the size of your responsibility, because you're spread way too thin.

2) Regarding releasing the code, is Clint on your team? If Clint is on your team, then Clint should be involved in doing code reviews, especially if he is a senior engineer. You should encourage your mentees to send code reviews to Clint where possible (don't overload him, but encourage them to involve Clint in the process). If Clint is not on your team, then unless the issues Clint finds are critical operation-breaking bugs, have him log them as bug reports; he's not on your team, he's not responsible for your product.

4) As for Clint missing "code review meetings": Firstly, I'm not sure what a "code review meeting" is. Code reviews are asynchronous operations: Developer submits code, code reviewer does review while developer does something else, review finishes, developer addresses comments, rinse, repeat. I don't know what a "code review meeting" is, it sounds silly. But beside that, if Clint is on your team, Clint should be attending team meetings. If Clint is not on your team, Clint does not need to attend team meetings. It's easy as that.

Somebody needs to define the "process", e.g. the sequence in which things happen.

As a developer, if I'm not the one defining the process then I'll do a code review (if that's my job, as expected by my manager) when the process tells me to and/or when someone asks me to.

I don't understand the bit in the question which says, "declined all our code review meeting invitations this month".

I'm not sure what a "code review meeting" is, by the way. My idea of a code review is:

1. Someone codes it


2. I review it (in my own time) and make notes


3. I meet with the coder to discuss my review

If I'm "senior" then perhaps I'm not listening to other people's review meetings.

To spare my time (reduce my effort) I like to review code after it has been tested. I might still find bugs (e.g. if the testing isn't perfectly complete), but it's easier to review code that works than code which doesn't. Reviewing code which doesn't work is called "development and debugging" and is more time-consuming.

Can you make "integration testing" any easier, more automated perhaps? So that you could do:

• Development


• Integration testing


• Review


• Fix review comments


• Integration testing again

Alternatively you merge before code review (if you trust that integration testing was sufficient without review), do the code review on the main branch, and use the code review comments as input for what you might want to improve on a subsequent iteration (a subsequent "sprint").

Pretty sure this guy is just telling you you're doing a bad job. That's what "it's not my job to teach your engineers" means. He's not interested in doing your job for you, he wants you to do it better. That's why he reviewed code destined for production (code you signed off on), not code in the pipeline. As for what to do about that, well, that's a topic for another question.

Beware.

✓ Senior Developer

✓ Finds issues in the code nobody else did

✓ Team discouraged because of the late reporting

✓ Reports something about "not his job" but reviewed anyway

✗ is setting up for a repeat of the same scenario

It doesn't look all that good for the team, or for the Sr. Developer either.

But I say to you beware. Make sure you understand all of the comments on that pull review. And I mean really understand. "That one's just wrong." doesn't count unless you really expended the effort on checking.

Probably there's nothing major waiting to be destructive to you. But there could be an extremely subtle data loss bug that he found that cannot be revealed in testing. You won't know the difference until you understand all the comments.

Addendum: To restore team morale, make sure get enough time to fix all of the issues the Sr. Developer found in that pull request that the rest of the team agree should be fixed.

The issue here is your workflow process is flawed. Peer Reviews should happen prior to integration testing for the simple fact that waiting until after testing is done can sow down the process. If the peer review finds issues that require changes and integration testing is already done more time will be needed to go back and redo all the testing after the changes are complete.

Ideally the developer should be writing and performing tests as they are developing the code and any final testing should be done after the code is peer reviewed to ensure that it is performing as expected. One thing to remember is that a peer review can spot bugs in the code before they can take down a system during integration testing.

You have two different kinds of reviews (three if you count your in-person meeting). That's not intrinsically bad, but it does mean you need to make your expectations very clear for each circumstance. I would create a pull request template for your final review that looked something like the following. This makes it very clear not only what is expected of this review, but also how to get your feedback addressed more successfully in the future.

<Summary of changes>

This is a pre-production review. Its purpose is to find major problems like:

• Merging mistakes


• Accidentally including an incomplete feature


• Showstopper bugs

Barring any of those sorts of problems, this pull request will be merged at <date and time>.

The individual design reviews, where we discuss readability, maintainability, and overall architecture, have already been completed. If you find those sorts of issues here, please open an issue or make the changes in your own pull request to development instead of cluttering this pull request. The design reviews were performed in the following pull requests:

• <Link to PR 1>


• <Link to PR 2>