Conversation
|
|
|
跟一下这轮 review comments,刚刚已经在
本地已跑过定向测试:
如果你更倾向于第 5 点走“fallback 回 etcd”而不是 fail-fast,我也可以再改。 |
# Conflicts: # src/sinks/topsql_data_deltalake/processor.rs
There was a problem hiding this comment.
claude:整体代码质量不错,测试覆盖充分,review comment 的处理也很到位(LRU cache、body 限制、fail-fast 配置校验等)。
另外有一个关于 TopSQLDeltaLakeSink::new 中 unsafe 块的建议:通过 raw pointer 从 Arc 中取出内部值比较脆弱,如果以后有人改了代码多加了一个 clone 就可能导致 UB。建议重构为:先创建 channel 和 shared state(Arc<Mutex<HashMap<...>>>),spawn task 时直接传这些 Arc,然后正常构造 Self 返回,完全不需要 unsafe。
|
跟进 review 处理内容:
本地定向验证:
|
|
codex: 关于 review body 里提到的 |
There was a problem hiding this comment.
claude:LGTM 🎉
上一轮的 review comments 都已修复:
- ✅
process_events_loop加了 max retry (5次) + exponential backoff (5s→60s cap) + 超限丢弃日志 - ✅
unsafe块移除,改用#[derive(Clone)]+sink.clone()(data 和 meta processor 都改了) - ✅
is_not_found_body收紧为"keyspace not found",支持纯文本和 JSON 格式,加了测试 - ✅
normalize_namespaces抽成公共函数normalize_namespace_list - ✅ shard config 初始化时读一次环境变量,缓存到 struct
- ✅ doc comment PLACEHOLDER 已补上
- ✅ FNV-1a hash 加了注释说明选择原因和跨服务契约
- Replace recursive JSON extraction with direct serde deserialization for ActiveTiDBAddress, removing ~100 lines of guessing logic - Remove redundant normalize_namespaces wrapper method - Change PdKeyspaceResolver::new to take Option<&TlsConfig> instead of owned Option<TlsConfig> to avoid unnecessary clones - Add debug_assert on replace_keyspace_route_segments to catch org_id/cluster_id containing path separators - Add TODO for resolve_keyspace concurrent cache miss dedup - Add comment explaining org=/cluster= path segment convention
Empty array from manager is a legitimate response (no active TiDB instances). Previously it was treated as an error while filter-to-empty after keyspace sharding was treated as Ok — inconsistent behavior. Now both cases flow through get_up_tidbs which logs and returns Ok, making the behavior uniform. Also removes the now-unused InvalidManagerResponse error variant.
… on retry process_events wrote dedup keys to LRU before flush_buffer persisted data to Delta Lake. If flush failed (e.g. PD keyspace lookup error), the retry path would find all keys already in LRU and silently skip them, causing permanent meta data loss. Fix: stage dedup keys in a pending_dedup_keys buffer during process_events (read-only LRU check). Commit to LRU only after flush_buffer succeeds. On failure, process_events_loop clears pending state so the cloned retry snapshot is re-processed cleanly.
# Conflicts: # src/sinks/topsql_data_deltalake/mod.rs # src/sinks/topsql_data_deltalake/processor.rs # src/sinks/topsql_meta_deltalake/mod.rs # src/sinks/topsql_meta_deltalake/processor.rs
…eyspace-cluster # Conflicts: # src/sources/topsql_v2/upstream/tidb/parser.rs
…ster module Deduplicate identical retry delay function and constants from both topsql_data_deltalake and topsql_meta_deltalake processors into common/keyspace_cluster.rs. Move the associated test as well.
No description provided.