review-crdb skill example: PR #161454 (engine separation ReadWriter)

review.md

Review: PR #161454 — kvserver: thread in correct engine when destroying and subsuming replicas

Summary

This PR replaces two uses of kvstorage.TODOReadWriter(b.batch) in replicaAppBatch.runPostAddTriggersReplicaOnly with a new b.ReadWriter() helper that correctly separates the state engine batch (b.batch) from the raft engine batch (b.RaftBatch()). This is part of the broader effort to logically separate the state and raft engines in the apply stack (issue #161059). The change is correct, small, and follows the pattern already established by splitPreApply on line 354 of the same file.

Overall assessment: looks good. Two minor observations below.

Findings

Comments

ReadWriter() method has no doc comment — pkg/kv/kvserver/replica_app_batch.go, line 198.

Every other method on replicaAppBatch has a doc comment. This new method should have one too. A brief comment explaining that it returns a kvstorage.ReadWriter routing state writes to b.batch and raft writes to b.RaftBatch() would orient future readers, especially given the ongoing engine-separation work where the distinction matters.

Severity: nit

Commit structure

The PR is split into two commits: one for DestroyReplica and one for SubsumeReplica. Both functions call the same destroyReplicaImpl under the hood, and the fix is identical (pass the properly-separated ReadWriter instead of TODOReadWriter). The first commit introduces the ReadWriter() helper and converts the destroy path; the second commit converts the subsume path. This is a clean split that makes each commit self-contained.

No issues here.

PR description

The PR body says only "See individual commits" and references issue #161059. For a change this small and straightforward, that is adequate -- the commit subjects clearly describe the two conversions.

No issues here.

Aspects skipped

• Redactability: no String(), Error(), or log output changes.
• Error handling: no new error creation or wrapping.
• Compatibility: no RPC, persisted format, or shared state changes. The ReadWriter construction is equivalent to the old TODOReadWriter when engines are not separated (since RaftBatch() returns b.batch in that case), so this is a no-op in production and only matters in test configurations with separated engines.
• Red/green testing: this is a mechanical cleanup of TODO markers, not a behavioral fix. No test changes are expected.