72

Over the summer I changed jobs from a big company as a senior engineer to a way smaller company as a principal engineer. Now I oversee about 20 jr-to-mid-level engineers working on 3 different reservation-booking systems. All the booking systems run on the same proprietary back-end framework, managed by another team that I don't oversee.

Last month on the night before launching a big release for one of the booking systems, a senior engineer ("Clint") on the back-end support team left a bunch of comments on a Pull Request we had opened to merge our release candidate into Master

The feedback ranged from somewhat helpful to somewhat unhelpful. We merged to Master anyway and the next day I asked if he could have reviewed that code earlier instead of waiting until after integration testing. When he left the feedback, it was the end of the day and we were preparing a ticket for our release engineer to deploy at 4:30am the next morning.

He told me that it's not his job to teach my engineers (he's right, it isn't). But it's hard for me to coach 20 engineers at once, even with them doing peer reviews and policing each other's code. I'm also worried my team was a little demotivated since they were unable to do anything to address the feedback.

We have another release scheduled right after we get back from Thanksgiving, and based on how Clint's declined all our code review meeting invitations this month, I think I'm going to see a repeat of the same thing in a couple days.

I can't tell if Clint really wants to help or just flex his ego. I would love his help coaching our junior developers, but the way he's doing it is unhelpful. I don't think my engineers will ever be able to catch everything Clint can.

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

EDIT: I am embarrassed I left this detail out but our engineers open pull requests from their feature branches to development which is where this feedback should go (on those pull requests)... when those changes are all ready to go to our production environment (after our QA engineers do integration testing and verify the changes are safe according to them) we open a PR to merge to Master after eng's approve the changes and agree we didn't introduce anything horrible and then deploy the next morning

Improve this question
10
  • 18
    What is Clint's role in the PR/merge process? Do you need his review to be allowed to merge? Is he just providing advice?
    – sleske
    Commented Nov 20, 2018 at 7:55
  • 10
    What's the overall process here? From what you have written, integration testing has been done and you have a PR open to merge to master, and you also imply you deploy from master as a result. Why is the PR done after the integration testing? How long was the PR open for? What other opportunities existed for feedback? What happens to your testing if a PR comment is made which highlights a fundamental issue? It sounds like there is a lot to be discussed here in a wider context with the entire team, and not necessarily an issue with Clint leaving comments on the PR imho.
    – user34687
    Commented Nov 20, 2018 at 9:29
  • 24
    Why did you wait until the last possible moment to merge? Is there a reason you didn't aim to merge a few hours earlier, at which point there probably would've been enough time to address any comments? If a pull request is open, it's expected that people will leave comments - that seems more like a problem on your side than their side. Also, how was he supposed to know he shouldn't comment? Note that if the comments themselves are requesting that you basically waste time changing things unnecessarily, you have a different question on your hands.
    – Bernhard Barker
    Commented Nov 20, 2018 at 9:43
  • 3
    It's a matter for OP to clarify, but to all of the commenters suggesting the PR should have been done earlier, if their process is anything like where I work (git flow) then the PR/merge to master is how the deploy to production is triggered. There would have been many earlier PRs to a develop (or trunk) branch for each individual feature, and PRs to various release branches for QA or other environments. Code reviews should have happened then, not on the final merge to master.
    – David Conrad
    Commented Nov 20, 2018 at 22:30
  • 2
    Code review his code review? Code Review Review: "Interesting feed back. Wrong in places. Needs more proactive approach. Practice better timing."
    – WernerCD
    Commented Nov 22, 2018 at 0:35

10 Answers 10

Reset to default
199

Unless Clint finds any major, “do not deploy”, bugs, I would simply thank him for his feedback and explain to him how you intend to address the points he raises (if valid) in future releases.

If he has a problem with this, then you have the opportunity to explain why it would be better for him to give feedback earlier.

Ultimately, it is his problem he is giving feedback so late. Don’t make it yours by taking the feedback as a personal / team failing.

Improve this answer
11
  • 44
    +1 this is the right answer. Feedback is feedback, but as the answer says unless "Clint" found some kind of blocker issue, the feedback will addressed later. If "Clint" is not happy about that, they can review the code earlier.
    – jcmack
    Commented Nov 20, 2018 at 5:54
  • 4
    Would you still say it's his problem if he didn't know they wanted to merge that evening?
    – Bernhard Barker
    Commented Nov 20, 2018 at 9:55
  • 18
    It's not his problem if this was his first real opportunity to give feedback. And even if it was, his problem is your problem is everybody's problem because you're all part of the same team and all wish it to succeed, no?
    – Lightness Races in Orbit
    Commented Nov 20, 2018 at 10:35
  • 5
    I fail to see how it's Clint's problem. Clint is a part of another team, it seems he is giving feedback independently of the process. The problem is that OP does not perceive it as very useful (because of timing). I think no one there has a clear idea what the code review should be about, or everyone has their own idea. It can serve many purposes. They should get on the same page.
    – luk32
    Commented Nov 20, 2018 at 11:59
  • 6
    I think this answer is right - I just don't see any 'problem' here. Maybe I'm misunderstanding? Clint provided new information, some of which OP believes is valuable. Awesome. That's always a good thing. If that info indicates that you can't deploy or merge or whatever, then it's BETTER to know asap. If that info indicates minor issues, awesome, log it and triage it appropriately. No problems here. If you want to be more responsive to Clint's feedback then sure, you need to get him involved earlier but I still see this as a perfectly fine situation as-is.
    – Rob P.
    Commented Nov 20, 2018 at 15:56
67

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.

Improve this answer
11
  • 22
    +1 - responsibility without the power to really do things is a problem I've seen more often than I'd like to.
    – Mołot
    Commented Nov 20, 2018 at 11:50
  • 9
    Given that "Clint" was apparently given less than a day to review man-weeks or perhaps man-months of work, this answer seems quite a way off the mark. Making the reviewer "responsible" for reviewing code faster than it's actually possible for them to review it is clearly unreasonable. And since Clint apparently did manage to leave some useful comments in the few hours he had, it seems all the more perverse to suggest that anything would be fixed if only the OP had the ability to force Clint to drop his own priorities and review or to blame Clint for being too slow.
    – Mark Amery
    Commented Nov 20, 2018 at 12:03
  • 8
    @MarkAmery I don't see in the question any indication that Clints time was limited by OP. If OP had the power to ask "how much time do you need for review?" and then power to say "OK, so you need to start at or before X date, because we need feedback before Y", then it would be OK. If OP can't do it, he shouldn't be held responsible for failure to respond to feedback. It should be management problem, not OP. It is not about blaming Clint, but about not requiring OP and his team to do the impossible.
    – Mołot
    Commented Nov 20, 2018 at 12:51
  • +1 Also, OP's team is free to address non-blocker feedback before the next release.
    – knallfrosch
    Commented Nov 20, 2018 at 14:15
  • @Mołot Peter Parker's Uncle Ben was close when he said "with great power comes great responsibility". They don't come together. They're the same thing. If I don't have the power to control how something is done, I am not responsible for how it's done wrong. Too many people try to divorce power and responsibility from one another, but it simply can't be done, and the attempts to do so fail. Whenever someone tries to hold me responsible where I have no power, I say "what exactly do you think I could have done differently to prevent ${BAD_THING}?"; the answer usually amounts to "nothing".
    – Monty Harder
    Commented Nov 20, 2018 at 20:10
8

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.

Improve this answer
3
  • To be more specific on the "busy" reason, Clint might feel like he's being dragged into a team and a workload that's not his responsibility when he already has enough (possibly even too much!) of his own to worry about. In that case, including him on bug reports and making other efforts to make him feel more involved/valued may actually work against the relationship.
    – jpmc26
    Commented Nov 22, 2018 at 1:42
  • IME support engineers are often over-burdened. It would he helpful to find out Clint's reasons for declining the code review invitations (he may simply be too busy). Letting Clint know that his contributions are valuable, however late, would be a good way to bring him closer to the OP's team. If Clint has capacity, but feels like he's not close enough to get involved earlier, he may just feel uncomfortable about treading on other people's toes.
    – linguamachina
    Commented Nov 22, 2018 at 21:06
  • Yeah, this is why you go out for coffee and talk it over. This one time he seems to have volunteered, so checking in and seeing if and how he'd like to contribute seems like the most direct way to work things out, and in an informal setting, he can blow off steam if he feels like he wasn't valued.
    – Joe McMahon
    Commented Nov 24, 2018 at 2:42
7

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.

Improve this answer
4

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.

Improve this answer
2
  • 7
    "Code review meetings" - We used to do these. Once a week, one of the lead devs would go through various recent merge requests and point out different examples of particularly good and bad code, and specifically what was good or bad about it. Then everyone would have a big discussion about it. It's a great way to distill knowledge from experience into something that juniors can understand and absorb.
    – industry7
    Commented Nov 20, 2018 at 18:42
  • 3
    Code reviews can be async. They can also be group meetings where the code is walked through. Turns out having a group can be more productive as different people catch different things.
    – DaveG
    Commented Nov 20, 2018 at 21:12
2

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").

Improve this answer
9
  • 4
    -1 for "code review should be done after testing". Some testing can be automated, but a lot of testing is manual (this is particularly true for frontend code, but can be true for backend code as well). Manual testing can take a long time; code reviews are generally short.
    – Ertai87
    Commented Nov 20, 2018 at 18:10
  • 1
    By "after testing" I mean, at a minimum, after rerunning automated smoke tests plus whatever you do to test that the new feature works as specified. Different systems have different amount of automation in their testing (and I said that if their testing was more automated then they might afford to do it more often).
    – ChrisW
    Commented Nov 20, 2018 at 18:33
  • Testing during development is different than testing in preparation for release and depending on the tests they could take a significant amount of time so you want to limit how often you run them.
    – Joe W
    Commented Nov 20, 2018 at 19:43
  • @JoeW Yes and "the night before a big release" does sound rather [too] late. I guess that "the feedback ranged from somewhat helpful to somewhat unhelpful" means he didn't find any show-stoppers, and so the OP was right to release on time when they did. And that's not all bad -- the fact a code review doesn't find show-stoppers isn't necessarily a bad thing. What is his job, if not to "teach my engineers"? Is it to just look at (i.e. to smoke) the changes in the integration branch before release?
    – ChrisW
    Commented Nov 20, 2018 at 20:06
  • 1
    I think you missed a critical issue with any testing before reviewing the code itself. If the integration/smoke tests will any signifiant amount of time to run does it make sense to run the tests and than do a peer review which could find issues requiring the tests to be run again? Wouldn't it make more sense to have the review done before hand so any potential issues could be found before testing happens so you minimize how often you need to run those tests?
    – Joe W
    Commented Nov 20, 2018 at 20:48
2

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.

Improve this answer
2
  • I honestly think this hits the nail on the head. Most people not on your team don't want extra work from your team. They just want your team to do good work so they don't have to think about it anymore.
    – jpmc26
    Commented Nov 22, 2018 at 1:40
  • I agree that this is a likely scenario. Since "Clint" has worked on the codebase longer, he likely knows it better as well.
    – Katinka Hesselink
    Commented Nov 22, 2018 at 13:34
1

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.

Improve this answer
0

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.

Improve this answer
9
  • "The issue here is your workflow process is flawed." I don't understand this perspective. To me it seems that the team is trying to do peer reviews early, and the issue is that Clint is ignoring them until the last minute. The workflow process seems fine, but Clint appears to be refusing to participate effectively.
    – DarthFennec
    Commented Nov 21, 2018 at 0:28
  • @DarthFennec Why would you ever want to do integration testing before doing a peer review of the code? To me that seems flawed as you will have to repeat any of the testing that was done if any issue are found with the code in the review.
    – Joe W
    Commented Nov 21, 2018 at 1:16
  • I'm not disagreeing with that. I'm saying OP doesn't appear to be disagreeing either. "the next day I asked if he could have reviewed that code earlier instead of waiting until after integration testing." It sounds like OP and his team are trying to do the peer review before integration testing, and the stated problem is that the reviewer (Clint) is acting against that workflow process.
    – DarthFennec
    Commented Nov 21, 2018 at 1:31
  • @DarthFennec Part of the problem here is the original question's lack of detail which lets us read very different underlying narratives into the story. Your reading, I guess, is that the team wanted Clint to review but he shirked his duties until the last minute. My reading was that Clint was uninvolved in the project and had no authority over it, then got ambushed on the day of the deadline with a giant PR of man-months of crap code that he'd had no prior chance to review, and then the OP got annoyed at the "late" feedback and merged it over Clint's objections.
    – Mark Amery
    Commented Nov 21, 2018 at 12:14
  • 1
    @MarkAmery "then got ambushed on the day of the deadline" does that not seems to contradict OP's behavior? Why would OP say "I asked if he could have reviewed that code earlier instead of waiting until after integration testing" if Clint was ambushed with the code after integration testing? Why would Clint's response be "it's not my job" instead of "I didn't know about it before integration testing"? I'm not saying you're wrong, I just don't understand how you reached your conclusion. I do agree that it would have been better if OP had been more clear about this, though.
    – DarthFennec
    Commented Nov 26, 2018 at 21:21
0

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>
Improve this answer

You must log in to answer this question.

Not the answer you're looking for? Browse other questions tagged .