Feature: rocksdb#142
Conversation
055960e to
9ee9097
Compare
da313b9 to
066cbdb
Compare
Introduce storage_rocksdb as a new pluggable storage backend alongside
storage_tcb/storage_tch. Behavior matches storage_tcb line-by-line for
set/get/remove/incr paths so the common storage test suite passes
identically (1618 tests, 100%).
Key points:
- storage_rocksdb.{h,cc}: RocksDB-backed storage with header cache for
deleted entries, snapshot-isolated iteration (localized ReadOptions
so Get() in set/remove/incr always sees latest state), and version
checking semantics matching storage_tcb.
- op_repl_sync_wal.{h,cc}: WAL-based incremental replication op.
- handler_dump_replication, op_meta, flared: wire up the new backend
and replication path.
- test/lib/test_storage_rocksdb.cc: run the common storage test suite
against the RocksDB backend.
- Build: configure.ac, Makefile.am, flake.nix, nix/default.nix,
cutter.patch, CI workflow, Dockerfile.test, BUILD.md,
ROCKSDB_REPLICATION.md.
066cbdb to
d046c67
Compare
The repartition test in flare-tests asserts a statistical bound (deviation < 0.3) on QuickCheck-generated key distribution and fails intermittently. Retry the nix test build up to 3 times (failed nix builds are not cached, so a retry re-runs the tests). Also stop running the workflow twice per PR commit (push + pull_request on the same SHA) by limiting push triggers to master, and cancel superseded in-progress runs.
Protocol (F1/F2/F3/F7): - Redesign repl_sync_wal as a push protocol: the replication source asks the destination for its recorded position (repl_sync_wal begin), streams its own WAL delta, and the destination applies it. The old integration ran the exchange backwards (the push handler pulled the destination's WAL) and never read the response, so incremental sync could never succeed and left unread bytes desynchronizing the shared connection. - Add repl_sync_wal seed: after a successful full dump the source records its lineage token and the covered WAL position on the destination, enabling incremental resyncs from then on. - Detect purged-WAL gaps in get_updates_since(): GetUpdatesSince() returns OK positioned at the first available batch even when the requested sequence was purged, so verify continuity instead of relying on IsNotFound(); fetch in bounded chunks (has_more) instead of materializing the whole retention window in RAM. - Guard all client/server stream parsing with try/catch, read exact sizes via readsize(), and bound batch sizes with a hard limit plus the configured ceiling on both ends; drain refused batches so the connection stays framed. Streaming throttle now runs on the sending side, making the rocksdb-wal-* ini options effective. - Filter reserved metadata keys out of applied batches so a peer's markers can never overwrite ours; stop applying after the first failed batch (no gaps). Accounting (F4): - Record resync success/failure from the actual outcome (op_set or iterator failure counts as failure; graceful shutdown is not counted), reset the failure streak on WAL success too, and account for early connect/iter_begin failures. Concurrency (F5/F6/F8): - Serialize iter_begin/iter_end with a dedicated mutex (busy return no longer leaks the whole-lock; concurrent iterations no longer race). - Protect _master_id with a mutex and return it by value. - truncate() now takes the whole lock exclusively and batches deletes; apply_batch* also runs exclusively. Behavior/perf (F8/F9): - incr matches storage_tcb: clamp to UINT64_MAX on overflow and reap expired records; store with a single Put instead of a second Get via set(). - count() is O(1) via an exact maintained record counter (persisted on clean close, re-counted after a crash, delta-tracked through WAL applies) instead of a whole-DB scan per stats request. Build (F10): - RocksDB is now an explicit opt-in: plain ./configure no longer links librocksdb just because it exists on the build host. - Add rocksdb to the dev shell. Tests (F11): - New test_op_repl_sync_wal.cc covers both protocol sides: framing, lineage mismatch, seed, abort/ack synchronization, oversized and malformed input, and a client-to-server roundtrip. - New storage tests: reopen persistence (data + repl_last_lsn), incr overflow clamp, count maintenance across restart/truncate/WAL apply, fetch budget progress, reserved-key filtering. Docs: ROCKSDB_REPLICATION.md updated to the push protocol; ROCKSDB_REVIEW.md added (review viewpoints, verdicts with evidence, fix status, verification checklist).
junjihashimoto
left a comment
There was a problem hiding this comment.
再レビュー (138286a) — F1〜F11 の修正確認 + 新規指摘
お疲れさまです。前回指摘 (ROCKSDB_REVIEW.md の F1〜F11) への対応をコードで追試しました。修正はいずれも本物で質が高いです:
- F1(応答未読/方向逆転/血統): push 型プロトコルへの再設計を確認。
begin/seedの全失敗経路(サーバ拒否・クライアント ABORT・mid-stream 失敗のドレイン・desync 時の再接続)で行同期が保たれていること、_read_final_result/_abort_streamで全応答をパースしていることを確認しました。 - F3: 4 箇所の
lexical_castが両側で try/catch 保護、BATCH サイズは uint64 で解析し 1GiB ハード上限 + 受信側 ini 上限で拒否、readsize()の使用も正しい(短絡読みなし)。 - F4: resync 計上の全 8 出口パスの分類が正しくなり、
_notify_resync_resultに集約。WAL 成功でストリークがリセットされ、graceful shutdown は計上されないことを確認。 - F5/F6/F7/F9/F10: iter ロックの原子化、
_master_idの mutex 保護 + 値返し、スロットルの送信ループ実効化、維持カウントの全変更パス整合、configure の明示 opt-in 化(-Iyes/includeバグ修正含む)をいずれも確認。 - F2: LSN 連続性チェックの境界演算にオフバイワンなし。ただしpurged 検出経路のテストが実在しない(下記コメント参照)。
ローカル (nix build .#test-flare-rocksdb) と CI の両方でテストスイートが green であることも確認しました。
ただし、再設計後のコードに 新規 critical が 3 件あります(いずれもマージ前に対応判断を推奨)
- C1: WAL 適用が宛先クラスタのルーティング/クラスタ内スレーブ複製をバイパス → 宛先が複数パーティション or 複数レプリカだとデータが正しく行き渡らないまま「成功」報告。
- C2: PR #140 で指摘した orphan purge の prepare 状態全消しが未修正のまま。
- C3:
master_idトークンが物理 DB のシーケンスドメインを識別できず、再構築でのトークン継承後に増分同期がサイレントにデータ欠落を起こす。
それぞれ該当行にインラインで詳細を書きました。major(ライブネス系のデッドロック/無限ループ、seed の非アトミック性、CI リトライのスコープ、テストギャップ)も併せてコメントしています。
優先度: C2 は小さな状態チェックの追加で防げる被害最大級のバグなので最優先。C1/C3 は設計判断が要るので、当面は「WAL 同期は宛先 1 ノード/1 パーティション構成限定」を明記し、検出時はフルダンプへ強制フォールバックする形が現実的だと思います。
| int partition = self.node_partition; | ||
| int partition_size = this->_cluster->get_node_partition_map_size(); | ||
|
|
||
| if (partition < 0) { |
There was a problem hiding this comment.
[C2 / critical] orphan purge が prepare 状態のノードで全データを消去しうる(PR #140 指摘の未修正持ち越し)
ガードが partition < 0 のみです。新パーティションのマスターが state_prepare/state_ready のとき node_partition = N ≥ 0 ですが、そのパーティションは _node_partition_prepare_map 側に存在し、get_node_partition_map_size()(:99)は active マップしか数えないため partition_size = N になります。すると resolve(h, N) ∈ [0, N-1] ≠ N が全キーで成立し、100% が orphan 判定 → 再構築中のデータセット全体が purge されます。prepare は数時間続く定常状態で node_map_version も不変なため token も失効しません。
修正方向: purge/scan の実行前に node_state == state_active かつ partition が active マップに存在することを確認して拒否する。
| partition, partition_size, (unsigned long long)nmv_start, rdb->get_master_id().c_str()); | ||
|
|
||
| if (partition < 0) { | ||
| // Node is not currently assigned to a partition (proxy, |
There was a problem hiding this comment.
[C2 関連] このコメントは「proxy/down/prepare の場合はここで弾く」と主張していますが、prepare 状態のノードは partition >= 0(prepare マップ側)なのでこの partition < 0 分岐には入りません。結果として prepare 中のノードで scan が全キーを orphan と報告し、有効な token を発行してしまいます(purge 側 C2 参照)。状態(state_active)チェックの追加が必要です。
|
|
||
| if (fail_reason.empty()) { | ||
| rocksdb::WriteBatch batch(string(batch_data, batch_size)); | ||
| if (rocksdb->apply_batch_with_lsn(batch, seq) < 0) { |
There was a problem hiding this comment.
[C1 / critical] WAL 適用が宛先クラスタのルーティングとスレーブ複製をバイパス
フルダンプ経路は op_set 経由なので宛先側で pre_proxy_write(所有ノードへのルーティング)と post_proxy_write(宛先ノード自身のスレーブへの複製)が働きます。一方この WAL 経路は受信エントリノードのローカル RocksDB に _apply_batch_filtered で直接適用するため、どちらも通りません。
- 宛先クラスタが >1 パーティション、またはエントリノードが所有者でない場合、キーが間違ったノードの storage に入り読めなくなります(ダンプ経路ならルーティングされていた)。
- 1:1 構成でも、宛先ノードのスレーブに増分が伝わらずサイレントに乖離し、宛先側フェイルオーバーで増分が全損します。
いずれのケースも resync は成功を報告しストリークをリセットします。ROCKSDB_REPLICATION.md にも宛先トポロジの制約が明記されていません。当面は「WAL 同期は宛先 1 ノード/1 パーティション限定」を制約として明記し、それ以外は検出時にフルダンプへ強制フォールバックするのが安全だと思います。
| // master_id_mismatch on every WAL attempt and burn cycles on | ||
| // redundant full dumps. | ||
| if (this->_storage->get_type() == storage::type_rocksdb) { | ||
| storage_rocksdb* rdb = dynamic_cast<storage_rocksdb*>(this->_storage); |
There was a problem hiding this comment.
[C3 / critical] master_id トークンが WAL シーケンスドメインを識別できない
ここでピアの master_id を別の物理 DB(独自のシーケンスカウンタを持つ)に採用しますが、対応するシーケンス位置は採用しません。下流(op_repl_sync_wal / handler_dump_replication)は master_id 一致を「宛先が記録した LSN は自分の WAL 上の位置」と解釈します。
具体シナリオ: パーティション分割で新マスター M1 が M0 から再構築して M0 のトークンを継承 → M0.id == M1.id。両者が mode=duplicate で同じ宛先に push。M1 が先にダンプ+seed(M1 ドメインの lsn)。M0 の次回 resync は id 一致を理由に M1 ドメインの lsn を「自分の位置」として信用し、その番号以降の M0-WAL エントリだけをストリーム → M0 はフルダンプせず、宛先は M0 のパーティションの大半を欠いたまま成功報告。lsn_ahead チェックは幸運な方向しか捕捉しません。ドキュメント記載のスレーブ昇格ケースでも同じドメイン混同が起きます。
修正方向: トークンは「血統」ではなく「物理 DB = シーケンスドメイン」を識別する必要があります。継承時に位置も併せて意味づけるか、DB ごとの epoch を導入し、ドメインが変わったら必ずフルダンプにフォールバックする。
| uint64_t from_lsn = dest_lsn; | ||
| uint64_t streamed = 0; | ||
| bool has_more = true; | ||
| while (has_more) { |
There was a problem hiding this comment.
[N1 / major] ストリーミングループが graceful shutdown で中断できず、追いつきループが無限化しうる
この送信ループ(_stream_batches)と受信側 _receive_batches(:405 の for(;;))は is_shutdown_request をチェックしません(op_dump.cc は毎キー確認、本ハンドラの Phase 3 も毎キー確認しています)。
- 受信側: bwlimit で絞られた数 GB のストリームが request スレッドを数時間
_receive_batchesに留め、データが流れ続けるため 10 分の read timeout も発火せず、宛先ノードの graceful shutdown が完了しない。 - 送信側: 書き込みレートが絞られた送信レートを持続的に上回ると、毎回の 64MiB フェッチが
has_more=trueで終わりwhile(has_more)が終わらない → resync が永遠に完了せず結果も返さず、ハンドラスレッドはソケットが死ぬまで shutdown を無視。
総バイト/総時間の上限もありません。各イテレーションで shutdown フラグを見る + 収束しない追いつきは N チャンクでフルダンプへ切る、を推奨します。
| if (rocksdb->set_master_id(this->_client_master_id) < 0) { | ||
| return this->_send_result(result_server_error, "seed_failed"); | ||
| } | ||
| if (rocksdb->set_repl_last_lsn(this->_seed_lsn) < 0) { |
There was a problem hiding this comment.
[M1 / major] seed が master_id と repl_last_lsn を非アトミックに永続化
set_master_id(:385)→ set_repl_last_lsn(:388)が別々の同期 Put です。1 つ目のコミット後に宛先がクラッシュ、または 2 つ目が失敗して seed_failed を返すと、(新 master_id, 古い lsn)が残ります。その古い lsn が別 lineage の残骸で source の最新シーケンス以下だと、次回同期でギャップをサイレントスキップします(C3 と複合)。
修正方向: 順序を lsn→id に入れ替えるか、両者を 1 つの WriteBatch で書く。バッチ適用側では既にこれを不変条件 S5 として実装済み(:1209 付近)なので、それに揃えるのが自然です。
| if (c->open() < 0) { | ||
| log_err("failed to connect to cluster replication server (name=%s, port=%d)", | ||
| this->_replication_server_name.c_str(), this->_replication_server_port); | ||
| this->_notify_resync_result(false); |
There was a problem hiding this comment.
[M5 / major→minor] 接続失敗が source の self-demote カウント対象になっています。このハンドラは健全で権威のある source マスター上で動くため、宛先クラスタが停止/到達不能な状態 + SIGHUP による cluster_replication 設定リロードが数回起きると(ストリークはインメモリで生き残る)、健全な source が threshold 3 で自分を state_down にしてしまいます。ROCKSDB_REPLICATION.md の意図表は op_set/イテレータエラーのみを失敗として挙げており接続失敗は含みません。接続失敗は計上しない、またはリロードとの相互作用を明記することを検討してください。
| if ((b & behavior_skip_timestamp) == 0 && e.expire > 0 && e.expire <= stats_object->get_timestamp()) { | ||
| log_debug("entry expired (key=%s, expire=%ld, timestamp=%ld)", | ||
| e.key.c_str(), e.expire, stats_object->get_timestamp()); | ||
| r = result_not_found; |
There was a problem hiding this comment.
[F8 残 / minor] get() で期限切れレコードを検出しても物理削除しません(r = result_not_found を返すだけ)。tcb の get はアンロック後に remove します(storage_tcb.cc 参照)し、compaction filter も設定されていないため、TTL 主体のワークロードでは期限切れレコードが上書き/明示削除されるまで残存し、ディスクと curr_items が単調増加します。add/replace の意味論自体は正しく出る(set 側で st_gone 扱い)ので破損ではなくドリフトですが、tcb との挙動差として記録しておきます。
| // verify the two reachable conditions: contiguous fetch succeeds, and | ||
| // a request beyond the latest sequence reports "up to date", never a | ||
| // bogus batch range. | ||
| void test_wal_get_updates_since_boundaries() { |
There was a problem hiding this comment.
[F2 テストギャップ / major] このテストは自身のコメントで「purged-gap 条件は live WAL では再現できない」と認めており、実際には contiguous-from-0 と beyond-latest しか検証していません。ところがチェックリスト(ROCKSDB_REVIEW.md)ではこのテストを根拠に F2 の purged 検出を ✓ としています。F2 はサイレントなデータ欠落だった S 級修正なので、その回帰テストが無いのは最大の乖離です。小さい wal_ttl/wal_size_limit で開き、書き込み後に Flush + WAL ファイルを削除してから LSN 0 で要求する、等で ERR_LSN_PURGED 経路を実際に踏めます。
| # The repartition test in flare-tests is statistical (random key | ||
| # distribution, fails when deviation >= 0.3), so retry up to 3 times. | ||
| run: | | ||
| for i in 1 2 3; do |
There was a problem hiding this comment.
[CI / major] この 3 回リトライは nix build 全体(flare ビルド + cutter 全ユニットテスト + flare-tests 統合テスト)を包んでいます。flaky なのは統計的な repartition テストだけのはずですが、この構造だと新規 WAL テストを含む全テストが 3 回チャンスをもらいます。30% の確率で落ちる新規レースが約 97% で CI を通過してしまいます。リトライは flare-tests 内の repartition ケースだけにスコープする(または unit テストと flare-tests を別ステップに分けてリトライは後者のみにする)ことを推奨します。同じ問題が :53 の rocksdb 側にもあります。
Fixes found reviewing the push-protocol redesign (138286a). See ROCKSDB_REVIEW.md section 4b for the full write-up. Critical: - C1: WAL apply bypasses the destination cluster's key routing and slave fan-out, so it is only correct for a single active partition with no slave. The destination now enforces this (cluster::is_wal_sync_destination_safe(), checked in begin/seed) and replies topology_unsupported otherwise; the source falls back to a routed full dump. op_repl_sync_wal now receives cluster*. - C2: orphan scan/purge guarded only on partition<0, so a node in state_prepare/ready (partition in the PREPARE map, absent from the active map) judged every key an orphan and could wipe the whole dataset mid-reconstruction. Both ops now additionally require state_active and partition < partition_size. - C3: the master_id token identified a lineage but not the physical DB (WAL sequence domain); a reconstructed master inheriting a peer's token could trust the peer's LSN as a position in its own WAL and skip updates. Introduce a separate immutable per-DB master_id and a repl_source_id marker recording which source repl_last_lsn belongs to; begin matches source_id against repl_source_id, and reconstruction no longer adopts the peer's token. Major: - M1: seed wrote master_id and repl_last_lsn as two Puts; a crash between them left a new source with a stale position. set_repl_source() now writes both in one atomic WriteBatch. - N1: neither stream side checked is_shutdown_request and a persistently- behind catch-up looped forever. Both loops now check shutdown each iteration; the source caps catch-up at max_fetch_iterations and falls back to a full dump. - N1': iter_begin held the wholelock rdlock until iter_end while the same thread's get()/remove() re-acquired it and truncate()/apply now take it as a writer — a deadlock/stall. Iteration no longer holds the wholelock at all (the RocksDB snapshot already gives a consistent view); _mutex_iter_lock still serializes iterations. - M5: connect/reconnect failures no longer count toward the source's self-demote streak (only real dump failures do). Minor / tests: - F8 residual: get() now physically removes an expired record after releasing its read locks (version-guarded), matching storage_tcb. - F2 test gap: add test_wal_get_updates_since_purged_detected, which flushes + deletes archived WAL files (flush_and_purge_wal_for_test) and asserts get_updates_since returns ERR_LSN_PURGED. - CI: split the workflow so unit tests run without retry and only the statistical flare-tests integration suite is retried. - Rewrote test_op_repl_sync_wal.cc for the source_id / topology model (mock_cluster); 20 wire-protocol tests pass. Updated ROCKSDB_REPLICATION.md (topology constraint, source vs master id).
138286a to
8253147
Compare
junjihashimoto
left a comment
There was a problem hiding this comment.
3rd review (8253147) — second-review 指摘への対応を検証しました
Address second-review findings の各修正を、ROCKSDB_REVIEW.md の記述ではなく実コードで追試しました。結論から言うと、指摘した項目はほぼ全て正しく修正されています。特に critical 3件はいずれも設計として妥当な直し方で、コードにも反映済みでした。素晴らしい対応です。
critical — すべて VERIFIED-FIXED
- C1(WAL適用が宛先ルーティング/スレーブ複製をバイパス):
cluster::is_wal_sync_destination_safe()を新設し、begin/seed両方で「active partition が partition 0 の1個のみ・スレーブ0・prepare partition 0」を検査。不適合ならtopology_unsupportedを返し、ソース側はclient_not_supportedに分類して正しくルーティングされる full dump にフォールバック。「1パーティションと報告するが実はスレーブがいる」バイパスも塞がれています。_receive_batches→apply_batch_with_lsnはこのガードを通る_run_server_begin以外から到達不可なことも確認しました。 - C2(orphan purge の prepare 状態全消し):
state_active必須 かつpartition < partition_sizeの二重ガードに。prepare/ready 状態のノードは両条件で拒否され、token は発行時・実行時の両方で再評価。具体シナリオ(partition 2 のマスターが prepare、active map size 2)で両条件とも拒否になることを確認。 - C3(master_id のシーケンスドメイン混同): 再構築時の peer-master_id 採用を削除し、
repl_source_idマーカーで「宛先の repl_last_lsn がどのソースのドメインか」を明示追跡。パーティション分割で M0/M1 が別 UUID になり、beginは宛先自身の master_id ではなく記録済みrepl_source_idと照合するため、クロスドメイン LSN を二度と信用しません。
major/minor — すべて VERIFIED-FIXED
- N1: 両ストリーミングループが毎イテレーションで shutdown 確認(受信側は mid-record で壊さず drain して枠同期維持)、追いつきは 64 イテレーション上限 + 64MiB/fetch で有界化。
- N1': iter_begin/next/end が wholelock を一切取らなくなり(スナップショットで整合性確保)、新設 writer(truncate/apply_batch の wrlock)との再帰 rdlock デッドロック連鎖が消滅。
- M1:
set_repl_source()が source_id + lsn を単一 WriteBatch(sync=true)で書き、非アトミック窓を解消。 - M5: 接続/再接続失敗は self-demote に計上せず、実ダンプ失敗のみカウント。意図もコメント化。
- F8残: get() が tcb 同様に期限切れレコードを delete-on-read(version_equal ガード付きで並行上書きに安全)。
- CI: ユニットテストはリトライ無しの別ステップになり、3回リトライは統計的 repartition 統合テストのみに限定。
新規バグの混入も特に警戒して見ました(F8 の skip_lock レース / shutdown による stream desync / 予約キーの WriteBatch 混入)が、いずれも正しくハンドリングされており新規バグなしです。
唯一の残提案(ブロッカーではありません)
F2 のテスト(test_wal_get_updates_since_purged_detected)だけ PARTIALLY-FIXED です。本番の連続性チェック(get_updates_since の3点の purge 検出)は正しく実装済みで、テストも実際に flush + WAL ファイル削除して呼ぶ本物の purgeになっています。ただしアサーションが ERR_LSN_PURGED と rc==0(contiguous)の両方を許容するため、環境が WAL を保持したままだと purge 検出分岐を踏まずに通ります。詳細は該当行のインラインコメントに書きました。
総評: 私の再レビュー指摘は critical 含めほぼ全項目が正しく対応されており、マージ可能な品質に達していると判断します。F2 テストのハードアサーション化だけ、余裕があれば対応を推奨します。
|
|
||
| // Two acceptable outcomes, both correct; a truncated stream (rc==0 | ||
| // with a first batch whose sequence > 1) is the silent-gap bug and | ||
| // must NOT happen. |
There was a problem hiding this comment.
[F2 / PARTIALLY-FIXED] このテストは実際に flush + WAL ファイル削除して purge を起こしており、以前の contiguous/beyond-latest しか見なかった版から大きく改善しています。ただしアサーションが ERR_LSN_PURGED(purge 検出)と rc==0(contiguous)の両方を許容する条件分岐なので、テスト環境が flush_and_purge_wal_for_test() 後も sequence 0 を WAL に保持していると、purge 検出分岐(updates.empty() を確認する側)を一度も踏まずに else 側で通ってしまいます。F2 はサイレントなデータ欠落だった S 級修正なので、その回帰テストが「purge しても検出しない」状態でも緑になり得るのは惜しいです。
提案: flush_and_purge_wal_for_test() が実際に early sequence を消したことを前提に、rc == ERR_LSN_PURGED をハードアサーションにする(purge 済みなら必ず検出、を保証)。もし環境依存で WAL 保持がありうるなら、purge 後に GetLatestSequenceNumber や WAL ファイルの実在を確認してから分岐させ、「purge が起きたなら必ず ERR_LSN_PURGED」という条件付きハードアサーションにすると、検出経路が毎回確実に実行されます。本番ロジックは正しいので、これはテストの厳密性だけの話です。
The test allowed both ERR_LSN_PURGED and a contiguous stream, so if the environment retained the WAL it passed without ever exercising the purge-detection branch — the exact regression F2 guards against could have gone green. flush_and_purge_wal_for_test() now probes GetUpdatesSince(1) after the flush + WAL-file delete and returns whether a purge was actually confirmed (early sequences no longer retrievable). When it was, the test hard-asserts ERR_LSN_PURGED; only if the WAL was genuinely retained does it fall back to the contiguous-stream check. Production continuity logic is unchanged — this is test strictness only.
Uh oh!
There was an error while loading. Please reload this page.