Giving and Receiving Feedback Questions

Direct answer

I write the comment around evidence and a question, in the pull request (PR) so there is a record, not around the person's seniority: describe the specific code path, the scale where it breaks, and ask about their assumptions. I mark it as a real concern, offer a way to test it, and if we still disagree I take it to a design conversation with the right owner. Respect shows in the evidence and the tone, not in staying silent.

How to word it

1. Name the behavior and the scale. "This loop issues one query per order row (the N+1 problem: one query to list N rows, then N more to fetch details). With 2,000 rows that is 2,001 queries per request."
2. Ask about assumptions, don't accuse. "What volume do we expect for this endpoint? If it stays small, this is fine."
3. Offer a test. "I can try it against the staging dataset (a test copy of realistic data) and share numbers." Measured results settle debates better than seniority.
4. Suggest a direction. "A single joined query (one query that combines the orders and customers tables), or batching the lookups (fetching many customers in one request instead of one at a time), would avoid it."
5. Label it, with a reason for the line. "Blocking if volumes are expected above ~100 rows; otherwise Consider." The ~100 is a team judgement, not a law. Illustrative arithmetic: if each query costs about 3 ms of round trip, 101 queries add roughly 0.3 s to a request, while 2,001 queries add roughly 6 s. Pick the line from your own latency budget and measured query time, and say so in the comment.

Follow-up

• If a reply is slow, a quick message: "Happy to pair on this if useful."
• If they disagree with evidence on their side, I update my view and say so.
• If we disagree and cannot resolve it: bring both views to the code owner (the person responsible for that part of the code) or the tech lead as a neutral question ("which risk is acceptable?") and write down the decision. Another compromise: merge behind a feature flag (a switch that turns the new code on or off without a new release) with a latency alert (a metric that warns if requests slow down), plus a follow-up ticket.
• Afterwards, keep the relationship warm: acknowledge when they were right about something else.

Worked example

Comment: "Possible performance issue at scale: get_orders calls fetch_customer inside the loop, so 2,000 rows means 2,001 queries (1 for the list plus 2,000 lookups). Is the expected volume small? If not, could we fetch customers in one query with a join? I can run this on the staging data and post the timing. I would treat it as blocking above roughly 100 rows and as a suggestion below that; the 100 is a team judgement, so tell me if the real volume sits on the other side."

Result: they point out the endpoint is limited to 50 rows by pagination (the API returns results in pages of at most 50). That is below the ~100-row line I set, so it is no longer blocking: 50 rows means 51 queries, not 2,001. I say, "Great, that changes it. Could we add a comment noting the limit?" and approve.

Trade-offs and pitfalls

• Deferring to rank lets known risks ship; ignoring rank creates friction. The evidence-first approach handles both.
• Do not make it public theatre: the PR is visible, so keep the tone neutral.
• Being wrong is possible. Ask first; they may know a constraint.

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

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 walk through a real review where I opened with the specific problem, worded it as a question with a reason and an offer of help, then followed up in person and checked that the fix landed. The PR (pull request) was fixed and the relationship stayed intact. Here is how I would tell it. The specifics are illustrative; in an interview use a real review of your own, or say plainly that you are describing how you would handle it.

Situation

A teammate about four months into the role, who had gone quiet in meetings after an earlier review thread got heated, submitted a change to a deployment configuration (a file defining how the service is rolled out). It set the memory limit (the most memory the service may use before the platform kills and restarts it) so low that the service would be restarted once traffic reached a busy-day level, and removed a readiness check (a probe that tells the platform whether an instance has started and can take traffic; a separate liveness check tells it whether a running instance is stuck). That was a real production risk, and I was the reviewer.

Actions I took

1. Decided what mattered. Two blocking points (memory limit, health check) and a few optional ones. I only wrote the blocking points in the first pass.
2. Worded it carefully. For example: "Blocking: with a 128Mi limit (128 mebibytes, about 134 MB of memory), I think the service will be killed once traffic rises, because the last load test, which simulated a busy day, showed it using about 300Mi. Could we raise it, or check the numbers together? Happy to pair." For the health check: "Could we keep the readiness check? It tells the platform an instance is ready to receive traffic. Without it the platform sends traffic to instances that haven't started yet."
3. Added real praise. "The rollout order is cleaner than the old file; thank you for splitting it up."
4. Talked in person after posting, in 10 minutes: "I left a couple of blocking comments. They are about the config, not about you. Want to go through them?"
5. Offered support. A pairing slot, plus a link to the team's config checklist.
6. Made sure it was acted on. I checked the PR the next morning; the new commit raised the limit and restored the check, and I approved.

Result

The PR merged within a day. Afterwards they messaged me: "Thanks, I would never have spotted the load test number. Can you look at my next draft before I open the PR?" They started sending drafts early, which told me the relationship was healthy.

If the issues had repeated

I would say so directly in a private one-on-one: "This is the second time the health check was dropped. What's making it easy to miss?" Then add it to the checklist or a pre-merge check. If it kept recurring after that, I would tell their manager what I had observed and what support I had offered, as a shared problem, not as a complaint.

What I would do differently

I would have talked to them before posting the comments, since I knew they were sensitive to written criticism.

Trade-offs and pitfalls

• Too gentle: the bug ships. Too blunt: the person stops sharing work early.
• Doing the fix myself would have been faster but they would not have learned.
• Praise must be true or it costs trust.

Direct answer

My default is asynchronous (written, on the pull request (PR), doc, or ticket) for specific, checkable feedback, because it is precise, linkable and gives the author time to think. I switch to live when the feedback is about direction or architecture, when the topic is ambiguous or emotionally loaded, or when an async thread has gone about two rounds of reply and counter-reply without converging. I make feedback get acted on by turning it into items with an owner and a date, writing the outcome of any live conversation back to the written record, and checking at a set point.

Choosing the channel

Making sure it is acted on

• Each item gets an owner, a due date, and a place (ticket or PR checklist), and I ask for a one-line reply.
• After a live review, I post the decisions and action items the same day; otherwise the conversation did not happen as far as absent teammates are concerned.
• At the next checkpoint I look at the list, not at my memory.

Reviewing SQL, ETL and dashboards

(ETL means extract, transform, load: the jobs that move and reshape data.) I use criteria, not taste, and put them in the review checklist:

• SQL: the output grain (what one row represents, for example one row per order), join correctness, row counts reconciled to the source, and readable formatting, enforced by a linter (a tool that automatically flags style problems) such as SQLFluff.
• ETL: safe to re-run without duplicating rows, automated data tests (for example the dbt tool's not_null check, which fails if a column has blanks, and unique check, which fails if a key repeats), and clear failure alerts.
• Dashboards: metric definitions written down, filters that behave as labelled, and the audience's question answered on the first screen.

A written review comment on a join might read: "This join to customers can return two rows per order when a customer has two addresses, so revenue is double counted. Row count before the join is 10,000 and after is 10,400. Please join on the primary address only, or aggregate first. Blocking, because the totals are wrong."

Tracking implementation uses the same method: checklist items in the PR or ticket, closed explicitly.

Example: a design review that changed a design, and the pushback

In a design review I flagged that a new feature chained three services synchronously (each call waits for the previous one to finish, so one slow or failed service stalls the whole request). If each service is up 99.9% of the time ("99.9% available"), the request only succeeds when all three are up, so the chain is about 99.7%:

0.999 x 0.999 x 0.999 = 0.997

That is roughly 26 hours of failure a year instead of 9, so one risky hop becomes everyone's outage. The owner pushed back: a queue (a buffer that lets work wait and be processed later, so the caller does not block) would cost two weeks. I asked what the real deadline was, separated must-fix from suggestions, and proposed a smaller step: make only one hop asynchronous, the one with the worst incident history (the weakest link in practice, even though the 99.9% figure above is a simplifying assumption of equal services), meaning it runs through the queue instead of making the caller wait. That leaves two synchronous services: 0.999 x 0.999 = 0.998, about 17.5 hours of failure a year instead of about 26. We agreed, and I wrote a short decision note with a trigger to revisit if volume doubled.

Pitfalls

• Two rounds of reply and counter-reply in a pull request (PR) thread without convergence, and certainly ten messages, is a signal to get on a call.
• A call without a written summary leaves the feedback unowned.