cockroachdb / cockroachdb/cockroach

kvnemesis: let operations carry their lock timeout instead of threading it through the applier

Open
#172,430 0 comments 0 reactions 0 assignees View on GitHub
A-kv-transactions C-cleanup O-agent P-3 T-kv
Dominant language
Go
Stars
32.5k
Forks
4.1k
PR merge metrics
PR metrics pending

Description

**Summary**

kvnemesis sets a `LockTimeout` on the batches issued by a fraction of transactions, so that operations blocking on a conflicting lock fail with a `WriteIntentError`. The timeout is chosen once per closure transaction and stored on `ClosureTxnOperation.LockTimeoutNanos`. To apply it, the applier extracts the duration and threads it as an explicit `lockTimeout time.Duration` parameter into `applyClientOp`, which sets it on every batch header it builds.

It would read more naturally for the individual `Operation` to carry its own lock timeout, so that `applyClientOp` reads `op.LockTimeoutNanos` off the operation rather than receiving it as a side parameter.

**Why this isn't a trivial field addition**

`Operation` is a `gogoproto.onlyone` union (see the generated `GetValue()`), so `onlyone` codegen assumes every field is a nil-comparable message pointer. Adding a scalar `int64 lock_timeout_nanos` to `Operation` produces uncompilable generated code:

```
operations.pb.go: invalid operation: this.LockTimeoutNanos != nil (mismatched types int64 and untyped nil)
```

because `onlyone` emits `if this.LockTimeoutNanos != nil { return this.LockTimeoutNanos }`. Every other `onlyone` message in the tree is message-only, so a scalar has never had to work there.

**Options**

- Drop `gogoproto.onlyone` from `Operation` and hand-roll `GetValue`/`SetValue`. This touches the core op-dispatch used throughout kvnemesis — largest churn.
- Add the timeout to each leaf op message (`GetOperation`, `PutOperation`, …). Removes the parameter but duplicates the field ~15 ways and pushes per-type handling into the applier.
- Bundle the applier's growing argument list (`inTxn`, `spIDToToken`, `lockTimeout`) into a small config struct. Removes the bare `time.Duration` parameter without proto changes, though it doesn't literally move the value onto the leaf operation.
- Leave as-is. The value already lives on the proto at its correct (transaction) level — the validator and formatter also read it from `ClosureTxnOperation` — and the parameter is just how a per-batch-header value chosen once per txn reaches the batch builder.

**Code references**

- `pkg/kv/kvnemesis/applier.go` — the `lockTimeout` parameter on `applyClientOp`; the `ClosureTxnOperation` case reads `o.LockTimeoutNanos` and threads it down.
- `pkg/kv/kvnemesis/operations.proto` — `ClosureTxnOperation.lock_timeout_nanos`; `Operation` with `option (gogoproto.onlyone) = true`.
- `pkg/kv/kvnemesis/validator.go`, `operations.go` — read the timeout from `ClosureTxnOperation` for validation and formatting.

Jira issue: CRDB-65666

Contributor guide

Open the contributing guide

Research direction

Start with pkg/kv/kvnemesis/applier.go and trace how ClosureTxnOperation.LockTimeoutNanos reaches applyClientOp. Read the Operation union and onlyone setting in pkg/kv/kvnemesis/operations.proto, then compare timeout handling in validator.go and operations.go. Done means the chosen design removes or relocates the side parameter without breaking generated code or the existing lock-timeout behavior.

Written by the indexing model from the issue text.

Assessment

Tech stack
go
Domain
databases, testing
Issue type
Refactor
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
48/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.