Speed up large raft apply conflict checks#1078
Conversation
📝 WalkthroughWalkthroughOCC競合検出をバッチ・単一イテレータ方式へ変更し、WAL復元のskip判定、volatile再生、raft適用高速化、トランザクションロック検査、バックグラウンド機能の環境変数制御を更新します。 ChangesMVCCストアのOCC処理
WAL冷スタート復元
トランザクションロック検査
バックグラウンド機能制御
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant ApplyMutations
participant ConflictChecker
participant PebbleIterator
participant WriteConflictError
ApplyMutations->>ConflictChecker: mutations/readKeys と startTS を渡す
ConflictChecker->>ConflictChecker: キーをソート・グルーピングする
ConflictChecker->>PebbleIterator: 最新コミット時刻を取得する
PebbleIterator-->>ConflictChecker: 可視コミット時刻を返す
ConflictChecker->>WriteConflictError: 最初の競合キーを渡す
sequenceDiagram
participant loadWalState
participant restoreSnapshotState
participant restoreSnapshotStateFromToken
participant reportColdStart
loadWalState->>restoreSnapshotState: committedTailIndex を渡す
restoreSnapshotState->>restoreSnapshotStateFromToken: トークンとコミット終端を渡す
restoreSnapshotStateFromToken->>restoreSnapshotStateFromToken: snapshot.Metadata.Index で判定する
restoreSnapshotStateFromToken->>reportColdStart: skip/execute とギャップ指標を報告する
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@codex review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request optimizes the conflict checking logic in the LSM store by sorting mutation and read keys and utilizing a single Pebble iterator to perform batched conflict checks. It also adds unit tests to verify that the batched checks preserve input order and handle large batches correctly. However, the reviewer points out that Optimistic Concurrency Control (OCC) conflict checks are not required at the store layer since the leader is responsible for issuing consistent timestamps, and suggests removing these checks entirely instead of optimizing them.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
store/lsm_store.go (1)
1431-1457: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winソート比較ロジックの重複を共通化できます。
sortedMutationConflictKeysとsortedReadConflictKeysはキー抽出部分のみが異なり、sort.Slice内の比較関数は完全に同一です。比較ロジックを共通ヘルパーへ切り出すと、将来ソート基準を変更した際の片方だけ更新漏れ（挙動乖離）を防げます。♻️ 比較関数の共通化案
+func sortConflictCheckKeys(keys []conflictCheckKey) { + sort.Slice(keys, func(i, j int) bool { + if c := bytes.Compare(keys[i].key, keys[j].key); c != 0 { + return c < 0 + } + return keys[i].index < keys[j].index + }) +} + func sortedMutationConflictKeys(mutations []*KVPairMutation) []conflictCheckKey { keys := make([]conflictCheckKey, len(mutations)) for i, mut := range mutations { keys[i] = conflictCheckKey{key: mut.Key, index: i} } - sort.Slice(keys, func(i, j int) bool { - if c := bytes.Compare(keys[i].key, keys[j].key); c != 0 { - return c < 0 - } - return keys[i].index < keys[j].index - }) + sortConflictCheckKeys(keys) return keys } func sortedReadConflictKeys(readKeys [][]byte) []conflictCheckKey { keys := make([]conflictCheckKey, len(readKeys)) for i, key := range readKeys { keys[i] = conflictCheckKey{key: key, index: i} } - sort.Slice(keys, func(i, j int) bool { - if c := bytes.Compare(keys[i].key, keys[j].key); c != 0 { - return c < 0 - } - return keys[i].index < keys[j].index - }) + sortConflictCheckKeys(keys) return keys }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@store/lsm_store.go` around lines 1431 - 1457, 共通のキー比較ロジックをヘルパーへ切り出し、sortedMutationConflictKeys と sortedReadConflictKeys の sort.Slice からそのヘルパーを再利用するよう更新してください。各関数のキー抽出と index の順序付けは維持し、重複した bytes.Compare と index 比較を一箇所に集約してください。
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@store/lsm_store.go`:
- Around line 1431-1457: 共通のキー比較ロジックをヘルパーへ切り出し、sortedMutationConflictKeys と
sortedReadConflictKeys の sort.Slice からそのヘルパーを再利用するよう更新してください。各関数のキー抽出と index
の順序付けは維持し、重複した bytes.Compare と index 比較を一箇所に集約してください。
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b6d60aa9-4135-4ab5-8512-cc610adf4f8a
📒 Files selected for processing (2)
store/lsm_store.gostore/lsm_store_txn_test.go
bootjp
force-pushed
the
fix/raft-apply-conflict-iterator
branch
from
38f8168 to
c6e5ddd
Compare
|
Addressed the high review note by documenting why store-level OCC remains required, then re-ran go test ./store ./kv and golangci-lint --config=.golangci.yaml run ./store ./kv --timeout=5m. @codex review |
bootjp
force-pushed
the
fix/raft-apply-conflict-iterator
branch
from
c6e5ddd to
7b3aaa1
Compare
|
Also addressed the sort-comparison duplication nit by adding a shared conflict-key sorter. Re-ran go test ./store ./kv and golangci-lint --config=.golangci.yaml run ./store ./kv --timeout=5m. @codex review |
|
Codex Review: Didn't find any major issues. 👍 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
bootjp
force-pushed
the
fix/raft-apply-conflict-iterator
branch
9 times, most recently
from
202e832 to
70f7913
Compare
TLA+ spec divergence review (auto-triggered)This PR touches files that the TLA+ safety spec has an anchor on (per Anchored files changed in this PR head (70f7913):
What to check, by subsystem:
If the change is correct but requires a spec update, edit @claude review please verify TLA+ spec divergence per the checklist above. @codex review please verify TLA+ spec divergence per the checklist above. |
|
To use Codex here, create a Codex account and connect to github. |
bootjp
force-pushed
the
fix/raft-apply-conflict-iterator
branch
from
70f7913 to
483103c
Compare
TLA+ spec divergence review (auto-triggered)This PR touches files that the TLA+ safety spec has an anchor on (per Anchored files changed in this PR head (483103c):
What to check, by subsystem:
If the change is correct but requires a spec update, edit @claude review please verify TLA+ spec divergence per the checklist above. @codex review please verify TLA+ spec divergence per the checklist above. |
|
To use Codex here, create a Codex account and connect to github. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 483103c25a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ts, exists, err := s.latestCommitTSWithIter(ctx, iter, current, &seekKey, &upperBound, preferNext, linearAdvanceLimit) | ||
| if err != nil { | ||
| return err | ||
| return nil, false, err | ||
| } | ||
| if exists && ts > startTS { | ||
| s.writeConflicts.record(WriteConflictKindRead, classifyWriteConflictKey(key)) | ||
| return NewWriteConflictError(key) | ||
| preferNext = iter.Valid() |
There was a problem hiding this comment.
When latestCommitTSWithIter misses current, SeekGEWithLimit can still leave the Pebble iterator valid at a key beyond the supplied limit; Pebble documents that this limit is best-effort. Setting preferNext solely from iter.Valid() then lets the next lookup start from that later key, so if the next mutation/read key sorts before that iterator position, conflictTSFromCurrentIter returns c > 0 and reports no version without seeking. In sparse batches this can skip a real latestCommitTS > startTS and allow an OCC-conflicting write/read set to commit.
Useful? React with 👍 / 👎.
TLA+ spec divergence review (auto-triggered)This PR touches files that the TLA+ safety spec has an anchor on (per Anchored files changed in this PR head (a57e6dd):
What to check, by subsystem:
If the change is correct but requires a spec update, edit @claude review please verify TLA+ spec divergence per the checklist above. @codex review please verify TLA+ spec divergence per the checklist above. |
|
To use Codex here, create a Codex account and connect to github. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a57e6dd1fe
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if mut == nil || len(mut.Key) == 0 { | ||
| continue | ||
| } | ||
| return s.committedVersionAtLocked(mut.Key, commitTS) |
There was a problem hiding this comment.
When callers pin CommitTS (supported by OperationGroup.CommitTS), a different raft entry can share the timestamp with an earlier entry. If its first mutation key already has a version at that timestamp, this returns true before checkApplyConflicts or applyMutationsBatch run, so a new entry like [a, c] after a prior [a, b] at the same commitTS is acknowledged as already landed, c is never written, and the applied index is advanced. The replay fast path needs a stronger same-entry check than the first key alone.
Useful? React with 👍 / 👎.
TLA+ spec divergence review (auto-triggered)This PR touches files that the TLA+ safety spec has an anchor on (per Anchored files changed in this PR head (37eb89b):
What to check, by subsystem:
If the change is correct but requires a spec update, edit @claude review please verify TLA+ spec divergence per the checklist above. @codex review please verify TLA+ spec divergence per the checklist above. |
|
To use Codex here, create a Codex account and connect to github. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 37eb89bea7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if iter.SeekGEWithLimit(seekKey, upperBound) != pebble.IterValid { | ||
| continue |
There was a problem hiding this comment.
When a batch lookup misses, SeekGEWithLimit is allowed to return IterValid on a key beyond upperBound; Pebble's next SeekGEWithLimit can then no-op if that overshot key is still below the next lookup's limit. In the transaction lock batch path, a missing earlier lock can therefore leave the iterator inside a later lock's key range, causing that later existing lock to be reported missing and allowing a raw/one-phase write to proceed through an active transaction lock. Treat out-of-range hits as forcing a real re-seek/reset before the next key.
Useful? React with 👍 / 👎.
| snapIndex := snapshot.GetMetadata().GetIndex() | ||
| decision, have := decideSkipOutcome(fsm, snapIndex) |
There was a problem hiding this comment.
When the durable FSM index is between the snapshot index and committedTailIndex, this now takes the skip path and returns have. On open, rawNodeAppliedForOpen only rewinds for config/volatile entries; data entries at indexes snapshot.Index+1..have are not passed to kvFSM.Apply, so their observeAppliedCommitTS calls are lost after a process restart even though their MVCC versions remain in Pebble. In that crash window the shared HLC can start below persisted commit_ts values and allocate duplicate/stale timestamps; the gate needs to replay/observe through the committed target or explicitly observe the skipped max commit timestamp.
Useful? React with 👍 / 👎.
TLA+ spec divergence review (auto-triggered)This PR touches files that the TLA+ safety spec has an anchor on (per Anchored files changed in this PR head (71455f8):
What to check, by subsystem:
If the change is correct but requires a spec update, edit @claude review please verify TLA+ spec divergence per the checklist above. @codex review please verify TLA+ spec divergence per the checklist above. |
|
To use Codex here, create a Codex account and connect to github. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
main.go (1)
352-355: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win環境変数名を構造化キーとして渡すことを推奨。
nameをメッセージ文字列に結合するのではなく、構造化キーとして渡すべきです。コーディングガイドラインではslogで安定した構造化キーを使用することが求められています。nameは常にハードコード定数のためログインジェクションの実害はありませんが、構造化ログとしてのクエリ性や一貫性が向上します。As per coding guidelines: "Use
slogfor logging and maintain structured keys (e.g.,key,commit_ts,route_id, etc.)"♻️ Proposed refactor for structured logging
- slog.Warn("invalid "+name+"; using default", - "value", raw, - "default", true, - ) + slog.Warn("invalid env var; using default", + "env", name, + "value", raw, + "default", true, + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@main.go` around lines 352 - 355, Update the slog.Warn call to keep the warning message static and pass name as a structured field using a stable key such as "name"; preserve the existing raw and default fields.Sources: Coding guidelines, Linters/SAST tools
main_background_env_test.go (1)
5-26: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winテーブル駆動テスト形式へのリファクタリングを推奨。
コーディングガイドラインでは
*_test.goファイルでテーブル駆動テストを推奨しています。現在の3つのサブテストは構造が重複しており、テーブル駆動にすることで簡潔になり、"true"や"0"などのエッジケース追加も容易になります。As per coding guidelines: "Unit tests must be co-located with packages in
*_test.gofiles; prefer table-driven test cases"♻️ Proposed table-driven test refactor
func TestEnabledEnv(t *testing.T) { - t.Run("unset returns default", func(t *testing.T) { - t.Setenv("ELASTICKV_TEST_BOOL_ENV", "") - if !enabledEnv("ELASTICKV_TEST_BOOL_ENV") { - t.Fatal("enabledEnv() = false, want true") - } - }) - - t.Run("false disables default true", func(t *testing.T) { - t.Setenv("ELASTICKV_TEST_BOOL_ENV", "false") - if enabledEnv("ELASTICKV_TEST_BOOL_ENV") { - t.Fatal("enabledEnv() = true, want false") - } - }) - - t.Run("invalid returns default", func(t *testing.T) { - t.Setenv("ELASTICKV_TEST_BOOL_ENV", "maybe") - if !enabledEnv("ELASTICKV_TEST_BOOL_ENV") { - t.Fatal("enabledEnv() = false, want true") - } - }) + tests := []struct { + name string + value string + want bool + }{ + {"empty returns default", "", true}, + {"false disables", "false", false}, + {"true enables", "true", true}, + {"zero disables", "0", false}, + {"one enables", "1", true}, + {"invalid returns default", "maybe", true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Setenv("ELASTICKV_TEST_BOOL_ENV", tt.value) + if got := enabledEnv("ELASTICKV_TEST_BOOL_ENV"); got != tt.want { + t.Fatalf("enabledEnv() = %v, want %v", got, tt.want) + } + }) + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@main_background_env_test.go` around lines 5 - 26, Refactor TestEnabledEnv into a table-driven test using a case slice containing each environment value and expected result, then iterate with subtests and set ELASTICKV_TEST_BOOL_ENV for each case before calling enabledEnv. Preserve the existing unset, "false", and invalid-value expectations while keeping the test easy to extend with additional boolean edge cases.Source: Coding guidelines
kv/fsm.go (2)
1515-1524: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
assertNoConflictingTxnLockの判定を共通化してください。prepareTxnMutationからまだ呼ばれているので dead code ではありませんが、decode→startTS/PrimaryKey比較 →NewTxnLockedErrorの流れはtxnLockConflictErrorと重複しています。単一件・バッチの両方で同じヘルパーを使う形に寄せると、今後の差分が減ります。🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@kv/fsm.go` around lines 1515 - 1524, assertNoConflictingTxnLock に重複している decodeTxnLock、startTS/PrimaryKey 比較、NewTxnLockedError の判定を削除し、単一件・バッチ双方が txnLockConflictError を利用するよう共通化してください。prepareTxnMutation を含む既存の呼び出し経路と、競合しない場合に nil を返す挙動は維持してください。
1432-1450: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value
ScanAtフォールバックの走査範囲を絞りたいGetAtBatchが使えない場合でも、ここはminLockKey..nextScanCursor(maxLockKey)をページングしており、対象外の txn ロックもまとめて読みます。targetsは exact key を持っているので、疎な mutation バッチではもっと狭い取得経路に寄せたいです。🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@kv/fsm.go` around lines 1432 - 1450, assertNoConflictingTxnLocksWithScan のフォールバック走査を、scanStart/scanEnd の広い範囲ではなく targets に含まれる正確なロックキーだけを取得する経路へ変更してください。GetAtBatch が使えない場合も対象キーを基準に取得し、取得した KV は既存の updateTxnLockCheckResult に渡して競合判定を維持してください。
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@store/lsm_store_applied_index_test.go`:
- Around line 114-115: Update the test setup after ApplyMutations to store
metaAppliedIndexBytes as a complete 8-byte little-endian value, such as
entryIdx-1, instead of []byte{0x01}; ensure the value represents an existing
older index so the stale comparison exercises the expected forward-progress
path.
---
Nitpick comments:
In `@kv/fsm.go`:
- Around line 1515-1524: assertNoConflictingTxnLock に重複している
decodeTxnLock、startTS/PrimaryKey 比較、NewTxnLockedError の判定を削除し、単一件・バッチ双方が
txnLockConflictError を利用するよう共通化してください。prepareTxnMutation を含む既存の呼び出し経路と、競合しない場合に
nil を返す挙動は維持してください。
- Around line 1432-1450: assertNoConflictingTxnLocksWithScan
のフォールバック走査を、scanStart/scanEnd の広い範囲ではなく targets
に含まれる正確なロックキーだけを取得する経路へ変更してください。GetAtBatch が使えない場合も対象キーを基準に取得し、取得した KV は既存の
updateTxnLockCheckResult に渡して競合判定を維持してください。
In `@main_background_env_test.go`:
- Around line 5-26: Refactor TestEnabledEnv into a table-driven test using a
case slice containing each environment value and expected result, then iterate
with subtests and set ELASTICKV_TEST_BOOL_ENV for each case before calling
enabledEnv. Preserve the existing unset, "false", and invalid-value expectations
while keeping the test easy to extend with additional boolean edge cases.
In `@main.go`:
- Around line 352-355: Update the slog.Warn call to keep the warning message
static and pass name as a structured field using a stable key such as "name";
preserve the existing raw and default fields.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 93ef96b5-403d-41fd-bc96-9ff9175e91b7
📒 Files selected for processing (13)
internal/raftengine/etcd/engine.gointernal/raftengine/etcd/engine_test.gointernal/raftengine/etcd/wal_store.gointernal/raftengine/etcd/wal_store_skip_gate_test.gokv/fsm.gokv/fsm_txn_test.gomain.gomain_background_env_test.gostore/lsm_store.gostore/lsm_store_applied_index_test.gostore/lsm_store_test.gostore/lsm_store_txn_test.gostore/mvcc_store.go
🚧 Files skipped from review as they are similar to previous changes (3)
- store/lsm_store_txn_test.go
- internal/raftengine/etcd/wal_store.go
- internal/raftengine/etcd/wal_store_skip_gate_test.go
| require.NoError(t, ps.ApplyMutations(ctx, muts, nil, commitTS, commitTS)) | ||
| require.NoError(t, ps.db.Set(metaAppliedIndexBytes, []byte{0x01}, pebble.Sync)) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# readAppliedIndexLocked が truncated 値を欠落扱いにするか確認
ast-grep outline store/lsm_store.go --match readAppliedIndexLocked
rg -nP -C4 'func .*readAppliedIndexLocked|len\(val\) < timestampSize' store/lsm_store.goRepository: bootjp/elastickv
Length of output: 1548
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# テストの該当範囲と、applied index の判定ロジックを読む
sed -n '1,220p' store/lsm_store_applied_index_test.go
printf '\n---\n'
rg -n -C6 'func .*shouldWriteAppliedIndexLocked|func .*readAppliedIndexLocked|LastAppliedIndex|metaAppliedIndexBytes|timestampSize' store/lsm_store.goRepository: bootjp/elastickv
Length of output: 21135
metaAppliedIndex には 8 バイトの値を書いてください
[]byte{0x01} は truncated 扱いで missing と同じ分岐に入るため、existing < appliedIndex の stale 比較を通せません。entryIdx-1 などの 8 バイト little-endian 値にして、古い index からの前進を直接検証してください。
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@store/lsm_store_applied_index_test.go` around lines 114 - 115, Update the
test setup after ApplyMutations to store metaAppliedIndexBytes as a complete
8-byte little-endian value, such as entryIdx-1, instead of []byte{0x01}; ensure
the value represents an existing older index so the stale comparison exercises
the expected forward-progress path.
Source: Coding guidelines
1 participant
Summary
Root cause
Large Raft apply batches repeatedly created Pebble iterators while checking each mutation key. During catch-up this made nodes spend excessive time in conflict checking before they could finish opening and serve traffic.
Tests
Author: bootjp
Summary by CodeRabbit