Code Style, Test Quality, and Building Trust

Trust in OpenSearch is not a feeling. It is an accumulated record of PRs that didn't waste a maintainer's time, didn't break a cluster, and didn't have to be reverted. This chapter is the mechanics of building that record: the formatting and static-analysis gates that you should never make a reviewer enforce by hand, the test discipline that proves your change works and keeps working, the CHANGELOG and scope habits that make you predictable, and the simple truth that sustained quality is what converts into review bandwidth and, eventually, a line in MAINTAINERS.md.

This is the everyday counterpart to PR Quality and the ground floor of the contributor → maintainer ladder.

The Automated Style Gates: Never Make a Human Enforce Them

A reviewer who has to comment "run the formatter" has already lost time, and you have already signaled that you didn't run the gate. The core repo enforces style mechanically; your job is to make all of it green before anyone looks.

The discipline is a fixed sequence — run it every time, in this order:

cd ~/OpenSearch
./gradlew spotlessApply        # 1. auto-format; commit the result
./gradlew spotlessJavaCheck    # 2. verify formatting is clean
./gradlew precommit            # 3. checkstyle, forbidden-APIs, headers, loggerUsageCheck, deps

Note: spotlessApply modifies your files. Run it, then git add and commit the result, so the formatted code is what you push. A common newbie failure is running spotlessJavaCheck (which only verifies) and being confused that nothing changed — use spotlessApply to actually fix it.

The precommit gate is the single check that bundles most of these. If ./gradlew precommit is green locally, the corresponding CI check (github review) will almost always be green too. There is no excuse for a red precommit check on a PR; it is entirely self-service.

Test Quality: The Part Maintainers Actually Trust

Style gates are table stakes. Test quality is where trust is built or destroyed, because a maintainer's deepest question about your PR is "if this breaks in six months, will a test catch it?" A change with a weak or absent test is a change the maintainer now has to worry about forever.

The properties of a test a maintainer trusts:

Use assertBusy, not sleep

OpenSearch is concurrent and asynchronous; "the thing happened" is rarely instantaneous. The deterministic way to wait is assertBusy, which retries an assertion until it passes or times out:

// GOOD: deterministic, fast when ready, bounded when not.
assertBusy(() -> {
    var resp = client().admin().cluster().prepareHealth().get();
    assertEquals(ClusterHealthStatus.GREEN, resp.getStatus());
});

// BAD: flaky on slow CI, slow on fast CI, proves nothing about *why* it waited.
Thread.sleep(5000);
assertEquals(ClusterHealthStatus.GREEN, /* ... */);

cd ~/OpenSearch
# See how the codebase waits on conditions:
grep -rn "assertBusy(" test/framework/src/main/java | head
grep -rln "Thread.sleep" server/src/test | head   # the anti-pattern to avoid in new tests

Prove your test actually tests

The cheapest way to earn a maintainer's trust is to demonstrate the test fails without the fix:

cd ~/OpenSearch
# 1) Stash your production change, keep the test:
git stash push -- server/src/main/java/...      # the fix only
# 2) Run the new test; it MUST fail:
./gradlew :server:test --tests "org.opensearch.your.NewTest"
# 3) Restore the fix; it MUST pass:
git stash pop
./gradlew :server:test --tests "org.opensearch.your.NewTest"

BWC tests for compatibility-sensitive changes

If your change touches the wire, index, or REST surface (maintainer-mindset), the trusted test is a round-trip serialization test — typically extending AbstractWireSerializingTestCase — exercising both the current and an older bwcVersion. See the serialization-BWC deep dive.

Flaky tests

If you hit a non-deterministic failure that isn't yours, don't silently re-run CI to bury it. Reproduce it from the printed -Dtests.seed=..., check for an existing flaky-test issue, and either fix it or mute it correctly:

@AwaitsFix(bugUrl = "https://github.com/opensearch-project/OpenSearch/issues/NNNN")

Never @Ignore a flaky test — that drops it silently with no tracking. The flaky-test roadmap stage covers this in depth.

CHANGELOG Discipline

Every PR to the core repo adds one line to CHANGELOG.md, in the correct category, under the unreleased section. This is not bureaucracy — it is the raw material for the release notes:

## [Unreleased 3.x]
### Fixed
- Fix DiskThresholdDecider off-by-one for relocating shards ([#NNNN](https://github.com/opensearch-project/OpenSearch/pull/NNNN))

The habits that matter: put it under the right heading (Added/Changed/Fixed/ Deprecated/Removed/Security); write it for a user reading release notes, not for yourself; and remember to move it to [Unreleased 2.x] when you backport. A PR missing its CHANGELOG entry is incomplete, and the checks will usually say so before a human does.

Focused PRs and Responsiveness

Two behavioral habits do more for your reputation than any individual piece of code:

• Focused PRs. One PR, one logical change. Do not bundle a refactor, a feature, and a reformat. A focused PR is faster to review, safer to merge (it squashes to one clean commit — see github review), and a better git bisect target later. If you catch yourself writing "and also" in the PR description, split it.
• Responsiveness. Address review comments within days, not weeks. Push fixes as new commits during review (they squash on merge); reply to each thread; if you disagree, say so with reasoning rather than going silent. A reviewer who knows you'll respond promptly invests more of their scarce attention in you.

These are precisely the behaviors in the "merged fast" column of the review chapter. None of them is about cleverness; all of them are about being a low-friction collaborator.

How Trust Compounds Into Maintainership

The mechanism is straightforward and entirely earned:

flowchart LR
    A[Green gates + strong tests + focused, responsive PRs] --> B[Reviewer spends less effort per PR]
    B --> C[You earn more review bandwidth, faster merges]
    C --> D[You start reviewing others, triaging, helping]
    D --> E[Maintainers trust your judgment across many changes]
    E --> F[Nominated and added to MAINTAINERS.md]

Each merged, clean PR is a deposit. Each revert, each "please run the formatter," each abandoned PR mid-review is a withdrawal. Maintainers grant the things you want — fast reviews, the benefit of the doubt on a design call, eventually merge rights — on the balance, not on any single transaction. The TSC-governance chapter describes the ladder; this chapter is how you actually climb the first rung of it.

Trust-Building Checklist

Run this on every PR before you ask for review:

• ./gradlew spotlessApply run and the result committed.
• ./gradlew precommit green (checkstyle, forbidden-APIs, headers, license, logger).
• ./gradlew assemble succeeds.
• Relevant tests added and run (:server:test --tests "..." / internalClusterTest).
• The new test fails without the production change (you verified this).
• No Thread.sleep in new tests; conditions waited on with assertBusy.
• Real assertions on real values — not "no exception thrown."
• BWC round-trip test added if the change touches wire/index/REST surfaces.
• CHANGELOG.md entry added under the correct section (and moved on backport).
• DCO sign-off on every commit (git commit -s → Signed-off-by:).
• PR is one focused logical change; title reads as the squash commit message.
• Issue linked; the why is explained for the reviewer.
• backport <branch> label added if the fix belongs in a maintenance line.

If every box is checked, you have removed every reason a maintainer could send the PR back before reading the logic — which is exactly how you earn the bandwidth to have the logic taken seriously.

Prove You Understand This

1. Which single ./gradlew task bundles most of the style/static gates, and what does spotlessApply do that spotlessJavaCheck does not?
2. Give three properties of a test a maintainer trusts, and the anti-pattern each one rules out.
3. Show, with commands, how you prove a new test actually fails without your production change.
4. Why is assertBusy preferred over Thread.sleep, and what's the correct way to handle a flaky test you didn't cause?
5. Where does your CHANGELOG entry end up, and what must you do to it when you backport?
6. Explain, in terms of deposits and withdrawals, how a track record of clean PRs becomes a line in MAINTAINERS.md.

This closes the Release & Governance Reality section. From here, take what you've learned into the capstone: a real issue, a reviewed PR, a backport, and a fix in a release — the whole machine, end to end.