Giving and Receiving Feedback Questions

Direct answer

I would first make sure I understand their objection by restating it and checking the facts, then reply with evidence, not opinion. I concede if their point stands up, hold if I have evidence and the risk is real, and escalate only after a direct conversation has not resolved it, while not letting the PR (pull request, a proposed code change awaiting review) sit.

Step by step

The example used below: the PR makes a payment call that retries on failure. The reviewer worries a retry could charge a customer twice and suggests an idempotency key (a unique token sent with each request so the server recognises a repeat and processes it only once). I think a simpler database rule already prevents it.

1. Pause and restate. "My understanding is that you are worried the retry loop (the code that repeats a failed call) can send duplicate charges. Is that right?" A hard comment is often a real worry stated badly.

2. Separate fact from preference. Is it a claim I can test (a bug, a measurable slowdown) or a style or design preference? Google's published code-review guidance says technical facts and data overrule opinions and personal preferences, and the style guide is the authority on style.

3. Check whether I am wrong. Reproduce their concern with a test. If it fails, concede and thank them.

4. Reply with evidence and the trade-off. "I tried the idempotency-key approach you suggested. It adds a table and a migration (a script that changes the database structure and must be rolled out carefully). My approach avoids that: the payments table already has a unique constraint (a database rule that rejects a second row with the same order id), and here is a test showing a retried call cannot create a second charge. Does that address it?"

   Check the claim against the failure that matters before sending it. A unique constraint only stops a second row in our own table. If the first call timed out after the payment provider had already charged the card, a retry would call the provider again, and no local constraint can stop that. In that case the reviewer is right and the idempotency key (sent to the provider, which many payment providers support) is the correct fix, so I would concede in step 3. My test must therefore simulate a timeout after the provider has processed the charge, not only a repeated local write. My position holds only if the charge record is created first, the provider call carries that record's id, and the provider deduplicates on it.

5. Offer a call if the second reply does not settle it. Text can read harsher than intended.

6. Escalate if still stuck. Google's guidance lists a broader team discussion, a tech lead, a maintainer of the code, or an engineering manager, and says not to leave the change waiting.

Decision table

Close the loop

After a decision, I thank the reviewer, and if I held my position I make sure the decision is recorded in the PR, so the next reader sees why.

What would change my call

A reproducible failing test, a production incident, or a teammate who owns the affected module disagreeing would move me toward conceding.

Direct answer

Good review feedback is specific, explains why, signals how important it is, and is about the code, not the author. The pattern I use is observation, issue, suggested fix, validation, with a severity label so the author knows what must change.

Severity labels (Google's engineering practices recommend this): Nit: a minor thing that technically should be done but will not change much; Optional or Consider: a suggestion; FYI: information for later. I add Blocking: for must-fix.

Three comments, worded out

1. Style nit

"Nit: d is hard to read here. daysSinceLogin would be clearer. Not worth holding up the change."

2. Design flaw with runtime impact

"Blocking: line 84 loads every order for the account and filters in memory. For accounts with tens of thousands of orders, each request holds all of them and gets slower as the account grows. Could we filter in the query (WHERE status = 'open', which asks the database to return only open orders instead of all of them)? Happy to pair on it (work on it together for a short session). A test with a large fixture (a prepared set of sample data, here an account with many orders) would confirm the fix."

3. Security or privacy concern

"Blocking: line 112 logs the whole request body, including email and shipping address. Logs are readable by more people than the database is. Please log only the order id. Can you also check the other handlers for the same pattern?"

What separates good from poor

A bug-shaped comment with repro steps (repro steps are the exact steps to reproduce the problem):

"Blocking: this returns a negative total for an empty cart. Input: a cart with no items and a $5.00-off coupon. Expected: total 0.00. Actual: total -5.00, because the discount is subtracted from a subtotal of 0 with no floor at zero. Could we clamp the total at 0.00, or reject the coupon when the item subtotal is 0?"

Short checklist before posting: Is it specific? Does it say why? Does it say how serious? Is there a next step? Would I be fine reading it aloud to the author?

Pitfalls: nitpicking dozens of style items buries the one real problem (let a linter, an automated style checker, handle style); asking questions as disguised orders; and reviewing by chat tone without ever saying what would make the change approvable.

Direct answer

Beyond correctness (does it run and produce the right numbers), I would review a training pipeline for seven things: data leakage, split quality, reproducibility, evaluation validity, consistency between training and serving (serving means using the trained model to make predictions on new data), operability, and tests. The split decides whether the score can be trusted, so it is checked next to leakage and evaluation, and tests keep all the other checks alive. I would phrase each comment as a specific observation, with the reason and a suggested change, and label how important it is.

What I look for

• Data leakage: information from the test data reaching training. Common case: fitting a scaler (rescales numbers to a common range) or imputer (fills in missing values) on the whole dataset before splitting it into training rows and test rows. Both learn statistics from the data, so fitting them before the split lets test-row information leak into training.
• Split quality: random splits for time-ordered or per-user data can leak. Is the split by user or by time where it should be? (A held-out split is the portion of data set aside and never used for training.)
• Reproducibility: fixed random seeds, pinned library versions (exact versions written down so reruns use the same code), the dataset version recorded, and configuration in a file, not hard-coded paths.
• Evaluation: is the metric the right one for the problem, computed on the held-out split only, and compared against a simple baseline?
• Training-serving skew: the same preprocessing code used at training and at prediction time, since a difference between how data is prepared in training and how it is prepared when the deployed model serves predictions silently changes the inputs the model sees.
• Operability: logging of metrics and parameters, checkpointing (saving progress periodically) so a failed run can resume, sensible failure on missing or empty data.
• Tests: a small fixture dataset (a tiny sample of data kept for tests) so the pipeline runs end to end in CI (continuous integration, automated checks on each change).

A leakage example

# split() is a stand-in for any helper that returns train and test rows
# Before: scaler sees the test rows
X_scaled = scaler.fit_transform(X)
X_train, X_test = split(X_scaled, y)

# After: fit on training rows only
X_train, X_test = split(X, y)
scaler.fit(X_train)
X_train = scaler.transform(X_train)
X_test = scaler.transform(X_test)

How I phrase the comments

This follows Google's published code-review guidance: comment on the code, not the developer, explain the reason, and mark nits and optional items.

Trade-offs

Not every item blocks. I would block on leakage and invalid evaluation, and leave the rest as optional or follow-up, so the review is fast and the author sees what matters.

Direct answer

I would comment on the claim, not the author, ask for the evidence as a question, say what would change my mind, and mark which comments block approval. I push back when the claim drives a costly or hard-to-undo decision, accept when it is cheap to reverse and the design includes a way to check it, and move to a call when the thread has gone two rounds without converging, when the tone is tightening, or when the disagreement is about meaning (what the claim or the goal actually is) rather than about facts a benchmark could settle.

Example

The design doc says: "The new cache will cut p99 latency (the time within which 99% of requests finish) in half." There is no benchmark.

Comment drafts

• Question about the evidence: "This claim is the main reason for adding the cache, so I want to be sure of it. What workload and data size was this measured on, and what is the baseline? If you have not measured yet, could we add a quick benchmark to the doc, or mark this as a hypothesis with a plan to test it?"
• Say what would settle it: "A comparison of cache-on vs cache-off on a replay of last week's traffic (recorded real requests sent through both versions) would convince me."
• Label severity: start with "Blocking:" for the claim, and "Nit:" or "Optional:" for minor edits, so the author knows what must change.
• Acknowledge what is good: "The invalidation section (how stale cached entries get removed) is clear, and the rollout plan is thoughtful."

Push back or accept

When to move to a call

1. After two rounds of replies that still disagree.
2. When the thread is getting longer or the tone is tightening, since text hides intent.
3. When the author is likely to be discouraged by a stack of comments.
4. When the disagreement is about meaning, such as what "latency" or "fast enough" refers to, not about a fact a measurement could settle. A call clears up definitions quickly, while more text rarely does.

Accepting an incidental claim without a check is safe only if the thread says it is unverified, so nobody later builds on it as if it were measured.

After the call, write the outcome back into the doc ("agreed to benchmark on replayed traffic, owner Sam, due Friday") so the decision is on record.

Written feedback on a research-style draft

The same structure fits a colleague's paper draft: a short summary of what you understood the claim to be, then strengths (specific), then concerns ordered by importance (for example: the baseline is weak, the claim is stronger than the experiment supports), each with a suggested check, in a respectful tone. A concern written out: "The comparison is against a model with default settings, so the gain may come from tuning, not the new method. Could you add a baseline that gets the same tuning effort? If the gap holds, the claim is much stronger." Starting with the summary shows the author whether you understood them, which often removes half the disagreement.

Direct answer

I would design a light process with four parts: (1) feedback attached to moments the work already has (a model review, a launch, a quarter end), (2) a short template that keeps it safe and specific, (3) every actionable item tracked with an owner and a date, and retros used to close recurring themes, and (4) a graduated, non-punitive response to teams that do not participate. Success is measured on participation, follow-through and recurring themes, by team, not by individual.

1. Timely: tie feedback to existing moments

• Trigger after milestones that ML teams already have: an experiment review, a model sign-off (the formal approval that a model is good enough to ship), a handover of a dataset or pipeline (passing ownership to another person or team), and a launch retro (a review meeting after release). Teams outside ML use their own equivalents, such as a design sign-off or a release retro.
• A short prompt goes to collaborators within a week of the milestone: "What should this person keep doing, and what is one thing to change?"

2. Safe: a small template and ground rules

• Two lines in SBI (Situation, Behavior, Impact) form plus one suggested next step. For example: "In the ranking-model review on 12 March, the ablation results were shared only as a screenshot (Situation and Behavior), so I could not check which features drove the gain and the sign-off waited two days (Impact). Next step: attach the notebook with the results."
• About the work and behavior, not the person's character. Feedback is developmental (meant to help the person improve) and kept separate from performance ratings and pay decisions, so people are candid.
• Attributed by default, meaning the giver's name is shown to the recipient, so the recipient can ask follow-up questions. For sensitive cases there is a manager-routed option: the giver tells their own manager or the recipient's manager, who passes the message on in person, still saying who it came from, rather than sending it anonymously.

3. Tracking follow-through

• Actionable items are written as: what, owner, due date, where tracked (the recipient's one-to-one (1:1) meeting notes or a team board).
• Teams review open items in their retrospective (a regular team meeting about how work went). If the same theme appears in two retros, it becomes a team improvement item with an owner, so recurring problems are solved at the system level.
• Measure follow-through as the share of items closed by their due date.

4. Chronic non-participation

• First, find out why: workload, unclear value, fear, or a lead who does not model it.
• Steps: a nudge, then a conversation between the team lead and their manager, then the participation expectation written into lead responsibilities. The aim is a fixed barrier, not a penalty.
• Report participation by team to leadership, never name individuals.

Worked example (illustrative numbers)

An organisation with 8 teams sends 24 feedback requests in a quarter (3 per team) and 15 are answered within 7 days: 15 / 24 = 62.5%. Of 12 actionable items raised, 9 are closed by their due date: 9 / 12 = 75%. That leaves 9 requests unanswered. Teams A, B and C answered 0, 1 and 1 of their 3 requests, so they account for 7 of the 9 unanswered; the other five teams answered 13 of 15. I meet those three leads, discover that one has no meeting slot after launches, and move the prompt into their launch retro. The next quarter I compare the same two rates for the same teams.

Pitfalls

• Over-engineered forms kill participation.
• Anonymous-only feedback cannot be followed up.
• Counting volume without checking follow-through rewards noise.