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:

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

1. 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.

2. Find assertBusy in the wild. grep -rn "assertBusy(" server/src/internalClusterTest | head — read how integration tests wait on cluster conditions without sleeping.

3. 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.

4. 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.

Validation / Self-check

You are done when you can answer these without notes:

1. 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?
2. Which base test class round-trips a Writeable, and why is its absence a blocker for a field-addition PR?
3. Name three things wrong with a public static final Map CACHE = new HashMap<>() in server code.
4. Why is Thread.sleep in a test a blocker, and what replaces it?
5. Which two process omissions in the example PR will make CI red on their own?
6. Why should this PR be split, and how would you phrase that ask?
7. 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.