Lab 2.4: Review It — Spot the Flaws in a PR

Background

Contributing is half of the job; reviewing is the other half, and the faster half to become valuable at. Maintainers spend more time reading PRs than writing them, and a contributor who reviews well is one a project trusts. This lab flips you to the maintainer's side: you are handed a plausible-but-flawed example PR — the kind that looks fine at a glance and fails on the things OpenSearch maintainers actually block on — and asked to enumerate every problem before you read the answer key.

The flaws are deliberately the recurring ones: a missing test, mutation of shared state, a backward-compatibility break in StreamOutput/StreamInput ordering, a Thread.sleep where assertBusy belongs, a missing CHANGELOG, and an unsigned commit. These are not exotic — they are the top reasons real PRs get "changes requested."

Why This Lab Matters for Contributors

Learning to spot these flaws is exactly how you learn to avoid them in your own PRs.
BWC and shared-state bugs are silent — they pass the author's happy-path test and break in production or on upgrade. Recognizing them on sight is a senior skill you start building now.
Reviewing well is the single fastest path to the trust that leads to maintainership (the path to maintainership).

Prerequisites

Lab 2.1–Lab 2.3 complete.
A reading acquaintance with Writeable/StreamInput/StreamOutput (from Lab 2.1; deep treatment in Level 9 and the serialization & BWC deep dive).

The Example PR

PR #99999 — "Add a priority field to FooStats and a helper to compute averages"

Description: "Adds a new priority field to FooStats so clients can sort stats. Also adds a small cache and a convenience method. Tested manually."

The diff (read it carefully before scrolling to the questions):

diff --git a/server/src/main/java/org/opensearch/foo/FooStats.java b/server/src/main/java/org/opensearch/foo/FooStats.java
index aaaaaaa..bbbbbbb 100644
--- a/server/src/main/java/org/opensearch/foo/FooStats.java
+++ b/server/src/main/java/org/opensearch/foo/FooStats.java
@@ public class FooStats implements Writeable {
     private final long count;
     private final long totalMillis;
+    private final int priority;

     public FooStats(StreamInput in) throws IOException {
+        this.priority = in.readInt();
         this.count = in.readLong();
         this.totalMillis = in.readLong();
     }

     @Override
     public void writeTo(StreamOutput out) throws IOException {
+        out.writeInt(priority);
         out.writeLong(count);
         out.writeLong(totalMillis);
     }

diff --git a/server/src/main/java/org/opensearch/foo/FooStatsCache.java b/server/src/main/java/org/opensearch/foo/FooStatsCache.java
index ccccccc..ddddddd 100644
--- a/server/src/main/java/org/opensearch/foo/FooStatsCache.java
+++ b/server/src/main/java/org/opensearch/foo/FooStatsCache.java
@@ public class FooStatsCache {
-    private static final Map<String, FooStats> CACHE = Collections.emptyMap();
+    public static final Map<String, FooStats> CACHE = new HashMap<>();

     public FooStats getOrCompute(String key, Supplier<FooStats> supplier) {
-        return supplier.get();
+        if (CACHE.containsKey(key)) {
+            return CACHE.get(key);
+        }
+        FooStats s = supplier.get();
+        CACHE.put(key, s);
+        return s;
     }

diff --git a/server/src/test/java/org/opensearch/foo/FooStatsCacheTests.java b/server/src/test/java/org/opensearch/foo/FooStatsCacheTests.java
index eeeeeee..fffffff 100644
--- a/server/src/test/java/org/opensearch/foo/FooStatsCacheTests.java
+++ b/server/src/test/java/org/opensearch/foo/FooStatsCacheTests.java
@@ public class FooStatsCacheTests extends OpenSearchTestCase {
     public void testCachePopulatesAsynchronously() throws Exception {
         FooStatsCache cache = new FooStatsCache();
         triggerAsyncPopulate(cache);
-        assertTrue(cache.size() > 0);
+        Thread.sleep(2000);
+        assertTrue(cache.size() > 0);
     }
 }

The commit that carries this diff:

commit 0badc0de
Author: A Contributor <contrib@example.com>

    Add priority to FooStats and a stats cache

(Note: no CHANGELOG.md change is in the diff, and the commit has no Signed-off-by trailer.)

Your Task

Before reading the answer key, write down every problem a maintainer would flag. Aim for at least seven. For each, note: what is wrong, why it matters, and what you would ask the author to do. Group them as: correctness, backward compatibility, testing, concurrency/state, and process. Only then continue.

(Your review notes here — do not skip ahead.)

The Maintainer's Review (Answer Key)

Here is the review a maintainer would actually leave. Each item is tied to an OpenSearch convention.

1. Backward-compatibility break in the wire format (BLOCKER)

The new priority field is read/written first, before the existing count/totalMillis:

this.priority = in.readInt();   // <-- new field read FIRST
this.count = in.readLong();

StreamInput/StreamOutput are positional — fields are read in the exact order they were written, with no field names. Inserting a new field at the front means an old node (which writes count, totalMillis) and a new node (which expects priority, count, totalMillis) will mis-parse each other's bytes: the new node reads the old node's count as priority, then reads garbage. This breaks every mixed-version interaction — rolling upgrades, cross-node transport — and corrupts data silently.

The convention: new fields are appended at the end, and their read/write is guarded by a version check so old nodes neither write nor expect them:

public FooStats(StreamInput in) throws IOException {
    this.count = in.readLong();
    this.totalMillis = in.readLong();
    if (in.getVersion().onOrAfter(Version.V_3_1_0)) {  // the version that introduces the field
        this.priority = in.readInt();
    } else {
        this.priority = DEFAULT_PRIORITY;
    }
}

@Override
public void writeTo(StreamOutput out) throws IOException {
    out.writeLong(count);
    out.writeLong(totalMillis);
    if (out.getVersion().onOrAfter(Version.V_3_1_0)) {
        out.writeInt(priority);
    }
}

Ask: "Append priority after the existing fields and guard read/write with getVersion().onOrAfter(...). Add a qa//serialization BWC test." This is the heart of Level 9 and the serialization & BWC deep dive.

2. No serialization round-trip test for `FooStats` (BLOCKER)

FooStats implements Writeable and just gained a field, but there is no test that round-trips it through StreamOutput → StreamInput. OpenSearch has a base class precisely for this: AbstractWireSerializingTestCase<FooStats>. Without it, the BWC bug in #1 would not have been caught by the author, and future field additions can break silently.

Ask: "Add a FooStatsTests extends AbstractWireSerializingTestCase<FooStats> with a createTestInstance() and (for BWC) a bwcSerializationTest across versions."

3. Mutable shared static state — a concurrency and correctness bug (BLOCKER)

public static final Map<String, FooStats> CACHE = new HashMap<>();

Three problems stacked:

static shared mutable state. A single process-wide HashMap shared across all instances and threads. OpenSearch is heavily multi-threaded (thread pools deep dive); concurrent put/get on a plain HashMap can corrupt the map (infinite loops, lost entries) and is a data race.
public exposes the cache for any code to mutate — no encapsulation, impossible to reason about.
Unbounded. It never evicts; it is a memory leak masquerading as a cache. Real caches in OpenSearch are bounded and often built on org.opensearch.common.cache.Cache.

Ask: "Don't use mutable static state. Make the cache an instance field; if it must be shared, use a bounded, thread-safe cache (org.opensearch.common.cache.Cache or at least a ConcurrentHashMap with an eviction policy). Keep it private."

4. `Thread.sleep` instead of `assertBusy` in a test (BLOCKER for tests)

triggerAsyncPopulate(cache);
Thread.sleep(2000);
assertTrue(cache.size() > 0);

Thread.sleep in a test is an anti-pattern OpenSearch reviewers reject on sight. It is flaky (2s may be too short on a loaded CI machine → spurious failure) and slow (2s wasted when the work finished in 5ms). The framework provides assertBusy(...), which polls a condition until it holds or times out:

triggerAsyncPopulate(cache);
assertBusy(() -> assertTrue("cache should populate", cache.size() > 0));

Ask: "Replace Thread.sleep(2000) with assertBusy(() -> ...). We do not allow Thread.sleep in tests — it is the #1 source of flaky-test issues." (Flaky tests and assertBusy are Level 5.)

5. Missing `CHANGELOG.md` entry (BLOCKER — CI will fail)

The diff adds a user-facing field (priority) but no CHANGELOG.md entry. The changelog CI check is red. Even ignoring CI, a new field is exactly the kind of change the changelog exists to record.

Ask: "Add a CHANGELOG.md entry under ## [Unreleased] → Added linking this PR."

6. Unsigned commit — DCO will fail (BLOCKER — CI will fail)

The commit has no Signed-off-by trailer. The DCO check blocks the merge.

Ask: "Sign your commits: git rebase --signoff upstream/main and force-push. The Signed-off-by must match your git identity."

7. Unfocused PR / scope creep (process)

The PR bundles two unrelated changes: a new FooStats.priority field and a new caching layer in FooStatsCache. These have different risk profiles (one is a wire/BWC change, the other a concurrency change), different reviewers, and different test needs. Bundling them means neither can be reviewed cleanly, and a problem in one blocks the other.

Ask: "Please split this into two PRs: one for the priority field (with BWC handling + a serialization test) and one for the cache (with a thread-safety design). They are independent."

8. The `priority` field is unvalidated and undocumented (minor)

priority is a raw int with no bounds, default, or Javadoc. What does a negative priority mean? What is the default for old data deserialized via the BWC path (#1)?

Ask: "Document the field, define a default (used in the pre-version-bump BWC branch), and validate the range if there is one."

The Review Rubric

Internalize the categories. A useful OpenSearch review checks, in roughly this order:

Category	What you check	Blocker?
Correctness	Does it do what the description claims? Edge cases?	Yes
Backward compatibility	Wire format (`StreamInput`/`StreamOutput` order + version guards), REST API, settings, index format	Yes — silent and severe
Concurrency / state	Shared mutable state, thread safety, no static mutable caches, proper `Closeable` handling	Yes
Testing	Is there a test? Right type (unit vs IT)? Serialization round-trip for `Writeable`? No `Thread.sleep` (use `assertBusy`)?	Yes
Scope	One logical change per PR; no drive-by refactors	Often — ask for a split
Process	DCO sign-off, CHANGELOG entry, SPDX header on new files, passing `precommit`	Yes — CI enforces most
Style / docs	Naming, Javadoc, message quality, follows house style	Usually non-blocking nits

Note: Lead with the blockers, not the nits. A review that opens with three style comments and buries a BWC break at the bottom helps no one. State the blocker, the why, and the concrete ask — the way the answer key above does. Tone matters; see GitHub review etiquette.

Implementation Requirements

Deliverables:

Your independent review notes, written before reading the answer key, listing at least seven problems grouped by category.
A diff of your notes against the answer key: which did you catch, which did you miss?
For each item you missed, one sentence on the convention you will remember.
A corrected version of the BWC read/write (append + version guard) written out from memory.
A one-paragraph review comment you would actually post on #99999 — blockers first, concrete asks, courteous tone.

Troubleshooting (Your Review, Not the Code)

"I only found four problems"

You likely caught the visible ones (CHANGELOG, DCO, Thread.sleep) and missed the silent ones (BWC ordering, static mutable state, missing serialization test). The silent ones are the valuable catches — train your eye on writeTo/StreamInput constructors and static mutable fields first.

"I flagged things that aren't actually wrong"

Over-flagging erodes trust as much as under-flagging. Before you post a comment, ask: is this a correctness/BWC/test blocker, or a personal style preference? Mark preferences as "nit:" so the author can weigh them appropriately.

"How do I know which version to put in the version guard?"

The next unreleased version on the line you target (the one in the ## [Unreleased] heading). The constant lives in the Version class — grep -rn "public static final Version V_" libs/core | tail. Exact handling is Level 9.

Stretch Goals

Find a real Writeable and audit its writeTo. Pick any class in :server implementing Writeable (grep -rln "implements Writeable" server/src/main/java | head) and confirm its constructor reads fields in the same order writeTo writes them, with version guards around the newer ones. This is what you will be doing for real in Level 9.
Find assertBusy in the wild. grep -rn "assertBusy(" server/src/internalClusterTest | head — read how integration tests wait on cluster conditions without sleeping.
Find a real BWC test. ls qa/ and open a rolling-upgrade or mixed-cluster test; see how the project proves old and new nodes interoperate.
Re-review your own Lab 2.3 change with this rubric. Does it have a test? A CHANGELOG? A sign-off? Is it scoped? Reviewing your own work this way before pushing is the habit that makes you a low-maintenance contributor.

Coding Exercises

Reviewing is a skill, but the deliverable of a good review is code: the test the author should have written, the corrected snippet, the bisect that pins the regression. Each exercise below turns one review instinct into something runnable. Locate real classes with rg/grep in your core checkout — never trust the illustrative Foo* names, which are invented for the lab.

(warm-up) Write the missing serialization round-trip test. Find a real Writeable value object in :server (rg -l "extends AbstractWireSerializingTestCase" server/src/test/java | head) and read one. Then pick a different Writeable (rg -l "implements Writeable" server/src/main/java | head) that has a thin or missing test and write a FooStatsTests extends AbstractWireSerializingTestCase<T> with a createTestInstance(). Verify with ./gradlew :server:test --tests "*YourTests*". This is the test flaw #2 in the answer key — now you can produce it, not just demand it.
(warm-up) Encode the BWC bug as a red test. Implement the broken version of the answer key's BWC change in a throwaway class: a writeTo/StreamInput pair that reads a new field first. Write a test that serializes with an old Version (out.setVersion(...)) and deserializes with a new one — assert the field survives. Watch it fail (mis-parsed bytes). Now fix it the answer-key way (append + getVersion().onOrAfter(...)) and watch it pass. You have reproduced blocker #1 as an executable proof.
(core) Replace Thread.sleep with assertBusy and prove the difference. Find a real assertBusy use (rg -n "assertBusy\(" server/src/internalClusterTest | head). Write a small OpenSearchTestCase that populates a structure on a background thread, first with Thread.sleep(2000), then with assertBusy(...). Time both (-Dtests.verbose); show the assertBusy version is faster and not flaky under a shorter budget. This makes blocker #4 concrete.
(core) Write the reviewer checklist as an executable lint. Turn the Review Rubric table into a runnable script (bash or a tiny Java/Python program) that, given a PR diff (git show <sha> > /tmp/pr.diff), flags: a new field inside a writeTo/StreamInput not guarded by a getVersion() check; a touched Writeable with no matching *Tests change; a Thread.sleep( in *Tests.java; a missing CHANGELOG.md hunk; a commit with no Signed-off-by. Run it against the example PR's diff and confirm it reproduces the answer key's blocker list.
(core) Bisect to find the regression a review missed. Pick any real bug-fix commit in core (git log --oneline --grep="fix" -20). Treat its parent as "good" and pretend the bug shipped. Write a one-line failing test that captures the bug, then git bisect start <bad> <good> driving it with git bisect run ./gradlew :server:test --tests "*YourTest*". Capture the commit git bisect blames and confirm it is the regressor. This is the skill the PR author's reviewer lacked — finding which change broke a thing.
(advanced challenge) Build a "review-as-code" harness for a field addition. Take a real Writeable in :server, add a new field to it correctly (append
- version guard + default), and ship the full review-grade package the example PR omitted: (a) a AbstractWireSerializingTestCase round-trip test; (b) a cross-version BWC assertion that serializes at the previous Version and deserializes at Version.CURRENT, proving the default applies; (c) a CHANGELOG.md entry under ## [Unreleased]; (d) a git commit -s so DCO is green. Run ./gradlew :server:test and ./gradlew precommit. You have now authored the PR you would have approved — the inverse of this whole lab. See the serialization & BWC deep dive and Level 9 for the version-guard mechanics.

Issues to Practice On

Reviewing real PRs and triaging real issues is how the rubric becomes muscle memory. Work the opensearch-project/OpenSearch tracker — the same repo whose conventions the answer key encodes.

gh issue list --repo opensearch-project/OpenSearch --label "good first issue" --state open
gh issue list --repo opensearch-project/OpenSearch --label "flaky-test" --state open
gh issue list --repo opensearch-project/OpenSearch --label "bug" --label "untriaged" --state open
# PRs awaiting review — the actual practice surface for this lab:
gh pr list --repo opensearch-project/OpenSearch --state open --label "backport 2.x" --limit 20
gh label list --repo opensearch-project/OpenSearch | grep -iE "test|bwc|backport|review"

Labels move; confirm on the tracker (gh label list) and pick the area label that matches the PR's subsystem (Search, Indexing, Storage, Cluster Manager).

Two representative patterns to practice on:

A flaky-test issue is almost always a Thread.sleep/timing assumption (the answer key's blocker #4). Reproduce by running the named test in a loop (./gradlew :server:test --tests "*X*" -Dtests.iters=50), rg the test for Thread.sleep( or unguarded async assumptions, replace with assertBusy, and open a PR with a CHANGELOG entry. This is a real, mergeable review-skill fix.
A PR with no test (extremely common on good first issue fixes): leave a review that doesn't just say "needs a test" but writes the test in the comment, named precisely (which base class, what createTestInstance returns). Lead with blockers, mark style as nit:, exactly as the answer key does.

Planted-bug drill. In a clean core checkout, open a real Writeable (rg -l "implements Writeable" server/src/main/java | head -1) and reorder one field in writeTo so it no longer matches the StreamInput constructor. Run its serialization test (./gradlew :server:test --tests "*<Class>Tests*") and watch it go red — note which assertion caught it. Now revert, and instead add a new field read before the existing ones with no version guard; observe that the plain round-trip test stays green (single-version) but a cross-version BWC assertion would have caught it. Add that BWC assertion. You have now planted, and caught, the exact silent flaw this lab trains you to spot on sight.

Etiquette: Claim an issue (comment to ask for assignment) before working it; reproduce before you fix; every PR needs a test + a CHANGELOG.md entry + a DCO Signed-off-by (git commit -s). When you review others, lead with blockers and a concrete ask. See community interaction.

Validation / Self-check

You are done when you can answer these without notes:

Why does inserting a new field at the front of a writeTo/StreamInput pair break backward compatibility, and what two things make a field-addition BWC-safe?
Which base test class round-trips a Writeable, and why is its absence a blocker for a field-addition PR?
Name three things wrong with a public static final Map CACHE = new HashMap<>() in server code.
Why is Thread.sleep in a test a blocker, and what replaces it?
Which two process omissions in the example PR will make CI red on their own?
Why should this PR be split, and how would you phrase that ask?
In a review, what do you put first — blockers or nits — and why?

You have now contributed a fix and reviewed one. That two-sided fluency is what Level 3 builds on as you move from workflow into the engine itself.

Open-Source Engineer & Contributor