Repository navigation
Code-health fixes: Raft DELETE ?rp= bug, dead code, duplicates, layering, write-path cost - #133
austin-barrington wants to merge 18 commits into
Conversation
…n one place In Raft mode mutation_request_from_statement used the database's default retention policy for DELETE and DROP SERIES, so a delete sent with ?rp=week removed rows from autogen. The non-Raft path built the same requests a second time inline, with the right policy. The builder now takes the request's retention policy and both paths use it.
Splits became metadata-only in 0.9.1, which left the post-commit reconciliation queue, its drain, re-home and push helpers with no caller; 17 #[allow(dead_code)] hid them (539 lines). They are deleted, with the scheduler's unused points_sink parameter and the tests of the queue itself. transfer_verified, ClearVerified and transfer_clear_proposals_enabled stay: routing and placement read the flag, splits and merges refuse flagged regions, and older Raft logs carry the op. A test pins that old config files setting the key still load.
Deletes items nothing in the workspace, its tests, benches or docs uses: the application/arrow_ingest module, drop_ch_objects_for_def, parse_line_body_to_points_with_capacity, ReplicationApplyQueue::with_workers, SharedShardMapPort, merge_field_disc, SanitizedIdent::sanitize, FieldValue::type_name, LocationCache::invalidate_all, with_concurrency_limit, with_arrow_inserts, default_pool_size (and the DEFAULT_POOL_SIZE it alone used), ArrowCache::insert_batch, with_message, RaftAdaptor, build_coalesced_fact_view_with_row_meta and the CLI's first_error.
hyperbytedb: tower, crc32fast, comfy-table, colored, simd-json. hyperbytedb-proxy: thiserror, tower. hyperbytedb-cli: anyhow, url. Cargo.lock loses only the six packages nothing else reaches; chdb-rust stays at 2.0.0.
MeasurementMeta::refuses_raw_writes(rp) replaces six copies of the same materialized / materialized_rp condition in the two ingestion services and the native adapter.
domain::chdb_naming::{string_literal, push_string_literal_body} replace the
query translator's quote_string body, query_service's
escape_clickhouse_string_literal and the native adapter's quoting. The
translator and the JSON subquery now escape control characters as the
native writer always did; ClickHouse reads both forms the same. The bind
parameter escaper is left alone: it writes InfluxQL, not SQL. A test pins a
tag value holding a tab, written through msgpack and matched in WHERE.
post_json and post_json_snapshot were line-for-line copies that differed only in the RPC error type; post_json is now generic over it.
…lding the Raft handle ports::leadership::LeadershipPort reports the current leader; HyperbytedbRaft implements it. RetentionService, ContinuousQueryService and the region heartbeat reporter take it instead of the Raft handle, and the two services' identical is_raft_leader bodies become one may_run gate.
ingest_metadata::parse_write_body replaces the identical parse-and-count blocks in IngestionServiceImpl::ingest and PeerIngestionService::ingest.
hash_password moves from adapters::http::auth_middleware to domain::user; the middleware re-exports it, and the two query services call the domain function.
ClusterRequest and ClusterResponse move to domain::cluster::raft_request (the adapter re-exports them); their JSON, the Raft log's wire format, is pinned by a fixture test. apply_create_database moves from the Raft state machine into application::schema_mutation_apply, its only caller.
ShardRoutingContext, the shard scheduler and the region heartbeat reporter hold Arc<dyn ShardMapPort> instead of Arc<RocksDbShardMap>; they only call snapshot() and apply_op(). Construction coerces the RocksDB map.
ports::replication_acks::ReplicationAcks has the six methods the flush service and drain read from the replication log; ReplicationLog implements it by delegation.
The binary point frame (POINTS_FRAME_MAGIC, is_binary_points, POINT_ENCODED_SIZE_HINT, encode_bincode_into) moves to domain::point_codec and ColumnarMsgpackBatch to domain::columnar_batch; the application modules re-export them, and the WAL batcher, peer client, points-sink port and chDB adapter use the domain paths. The cluster bootstrap, leader monitor and Raft formation build concrete adapters and take the HTTP AppState, so they move from application::cluster to a top-level composition module.
split_field_type_conflicts runs on every line-protocol batch, before the schema cache. It re-inserted every field of every point, cloning the key each time; it now widens known fields in place and clones only new keys. New bench line_protocol_field_types/split_field_type_conflicts_5000x10 (5000 Telegraf cpu points x 10 fields): 4.80 ms -> 2.82 ms per batch (-41%, p < 0.05). A test pins a field widening then conflicting within one batch.
ingest_metadata::BatchSchema (per measurement: widened field types and tag keys) is built once per client write and shared by the name-collision guard (check_schema) and prepare_batch_metadata_with_schema; the guard no longer walks every point, and takes its read lock once per measurement. New bench line_protocol_name_guard/name_guard_5000x10: 1.87 ms -> 1.25 us per batch.
- A Raft-mode test drives DELETE ?rp=week through execute_query on a real single-node Raft and checks rows go from week, not autogen; it fails if the retention policy is dropped at the call site. - The widen-then-conflict test adds an unsigned point, which is dropped only if the integer->float widening was kept. - Docs and comments describe the moved modules at their new paths, and say plainly that nothing sets or clears transfer_verified any more and that transfer_clear_proposals_enabled has no effect.
PR SummaryMedium Risk Overview Cluster correctness: Raft and non-Raft cluster paths now build Sharding / dead code: ~540 lines of unused post-split transfer reconciliation come out of the shard scheduler (including Consolidation: MV destination checks use Layering: Password hashing, Write path: Ingest builds a Reviewed by Cursor Bugbot for commit b144dbf. Configure here. |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: ef6431e5-98fb-44d1-ae62-8064494901f4) |
Stacked on #132. Implements
docs/superpowers/plans/2026-11-03-code-health-fixes.md(included in this PR with the five follow-up plans), one commit per task.Bug
DELETE/DROP SERIESignored?rp=and deleted from the default retention policy. Both cluster paths now build the mutation in one place, using the request's retention policy. A test runs the DELETE through a real single-node Raft and checks that rows go fromweekand notautogen.Dead code and dependencies
~540 lines of unused split-transfer reconciliation removed from the shard scheduler, along with its 17
#[allow(dead_code)]. Thetransfer_verified/ClearVerified/transfer_clear_proposals_enabledfields, op and config key stay. Old config files still load, and a test pins that.Other unreferenced code removed:
application/arrow_ingestmodule;drop_ch_objects_for_def,merge_field_discandRaftAdaptor.Unused crates removed:
Cargo.lockloses only the six packages nothing else reaches, and chdb-rust stays at 2.0.0.Duplicates
MeasurementMeta::refuses_raw_writes; it replaces 6 copies.LeadershipPortreplaces the copies ofis_raft_leader. Retention, continuous queries and the heartbeat reporter no longer hold the Raft handle.Layering
domain::user.ClusterRequest/ClusterResponsemove todomain::cluster::raft_request. Their JSON is the Raft log's wire format, and a fixture pins it.Arc<dyn ShardMapPort>instead ofArc<RocksDbShardMap>.ReplicationAcksport replaces the concreteReplicationLogin the flush and drain code.ColumnarMsgpackBatchmove to the domain.application::clustertocomposition.PeerClientand Raft proposals, which2026-11-03-peer-rpc-port.mdcovers.Write-path cost (5,000 Telegraf
cpupoints × 10 fields; new criterion benches)split_field_type_conflicts: 4.80 ms → 2.82 ms per batch. It now widens types in place instead of cloning every field key.Review
A fresh reviewer checked the whole branch and found no correctness or compatibility problems. Their findings and how each was handled:
Tests
cargo fmt --checkandcargo clippy --all-targets -D warningspass.cargo test --lib: 881 pass.One run hit a pre-existing flake,
sharding_cluster_integration::percentile_answers_when_groups_are_whole_seriesreturning a 500 under full parallel load. It passed 3/3 alone and on a full rerun.