Section-by-section review notes¶
Rolling record of a full-monorepo pass against code-review-rubric.md. Severity: blocker, major, minor, nit. This document is updated when architecture changes; PRs should add a row or subsection when they materially alter a layer.
1 — Product / architecture¶
Paths: README.md, docs/, config.toml.example
| Finding | Severity | Notes |
|---|---|---|
| README architecture diagram matches hexagonal layout | nit | Aligns with src/lib.rs modules |
| Doc index in README | ok | Table points to configuration, clustering, operations |
| Engineering rubric linked from developer guide + README | ok | Contributing: engineering review |
Bloat: None identified; docs/ topics are distinct (deep dives vs. operations).
2 — Domain, errors, config¶
Paths: src/domain/, src/error.rs, src/config.rs
| Finding | Severity | Notes |
|---|---|---|
HyperbytedbError as library boundary | ok | thiserror variants used across adapters |
| Config validation | should-verify per change | Large HyperbytedbConfig; new fields need env + TOML + docs |
Tests use unwrap on JSON in config.rs | nit | Confined to #[cfg(test)] blocks |
Bloat: None.
3 — Ports¶
Paths: src/ports/
| Finding | Severity | Notes |
|---|---|---|
Trait objects Arc<dyn …Port> | ok | Consistent async patterns |
Ingestion port columnar-ingest cfg | ok | Feature isolated in ingestion.rs |
Bloat: None.
4 — Adapters — storage¶
Paths: src/adapters/storage/
| Finding | Severity | Notes |
|---|---|---|
Parquet compression level ZstdLevel::try_new(1).unwrap() | minor | Acceptable if ZSTD level 1 always valid; consider expect with reason |
| Object store + local layout | ok | object_store feature set matches S3 path |
Bloat: None.
5 — Adapters — WAL / metadata¶
Paths: src/adapters/wal/, src/adapters/metadata/
| Finding | Severity | Notes |
|---|---|---|
| Batching WAL | ok | Backpressure-sensitive; review when changing flush |
Bloat: None.
6 — Adapters — chDB¶
Paths: src/adapters/chdb/
| Finding | Severity | Notes |
|---|---|---|
| Pool + query adapter | ok | Timeouts should stay aligned with HTTP timeouts |
Bloat: None.
7 — Adapters — HTTP¶
Paths: src/adapters/http/
| Finding | Severity | Notes |
|---|---|---|
| Router builds centralized | ok | router.rs |
| Auth middleware | ok | Review when adding routes |
Bloat: None.
8 — Application services¶
Paths: src/application/
| Finding | Severity | Notes |
|---|---|---|
Columnar msgpack tests use unwrap/expect | nit | Inside #[cfg(test)] in columnar_msgpack.rs |
| Service orchestration | ok | Compaction, flush, retention, CQ split |
Bloat: None.
9 — Cluster / replication¶
Paths: src/cluster/
| Finding | Severity | Notes |
|---|---|---|
SyncClient held unused storage: Arc<dyn StoragePort> | major (cleanup) | Removed: field was never read; only added constructor cost |
| Raft / sync / HH | ok | Large surface; e2e in Go covers multi-node |
Bloat addressed: dead storage field on SyncClient (see git history / PR description).
10 — InfluxQL¶
Paths: src/influxql/
| Finding | Severity | Notes |
|---|---|---|
Large to_clickhouse.rs | nit | unwrap largely in tests at file tail |
| Parser + digest | ok | Fuzzing not in-repo; optional future |
Bloat: None.
11 — Bootstrap + entry¶
Paths: src/bootstrap.rs, src/main.rs, src/bin/
| Finding | Severity | Notes |
|---|---|---|
main.rs very large | minor | CLI + serve + cluster lifecycle; extract modules over time per rubric Layer B |
bootstrap.rs metrics expect | nit | Single global recorder install |
Bloat: None in bins; debug/backfill bins are intentional.
12 — Benches / examples / scripts¶
Paths: benches/, examples/, scripts/
| Finding | Severity | Notes |
|---|---|---|
| Columnar bench gated | ok | required-features = ["columnar-ingest"] |
scripts/bench-columnar-http.sh | ok | Thin wrapper |
Bloat: None.
13 — Rust tests¶
Paths: tests/
| Finding | Severity | Notes |
|---|---|---|
compat/ modular | ok | main.rs harness |
| JS + shell helpers | ok | Used by load.sh and Kind setup |
| Overlap with Go e2e | minor | Go owns multi-node; Rust compat owns v1 API matrix — keep both per rubric Layer D |
Bloat: None removed.
14 — Go e2e + operator¶
Paths: tests/e2e/, hyperbytedb-operator/
| Finding | Severity | Notes |
|---|---|---|
Operator bin/* gitignored | ok | Local controller-gen; not repo bloat |
| Kubebuilder discipline | ok | See AGENTS.md |
E2E not in main Rust ci.yml | minor | Policy: run on demand or add job if regressions slip |
go test ./... in operator | env-dependent | Controller tests need envtest (kubebuilder etcd binary); failure without /usr/local/kubebuilder/bin/etcd is expected locally |
Bloat: None in tracked tree.
15 — CI/CD + containers¶
Paths: .github/workflows/, Dockerfile, deploy/, docker-compose.yml
| Finding | Severity | Notes |
|---|---|---|
RUSTFLAGS: -D warnings | ok | Strict |
libchdb install in CI | ok | Required for link |
| Container workflow on PR builds | ok | Push only when not PR |
Bloat: None.
Dependency / static analysis (de-bloat pass)¶
| Check | Result |
|---|---|
cargo machete | No unused dependencies reported (2026-04-13) |
.gitignore hygiene¶
| Finding | Severity | Notes |
|---|---|---|
Duplicate patterns (/.vscode/, .DS_Store, tests/tmp/, *~, *.d, *.test) | nit | Consolidated in root .gitignore |
De-bloat changelog (this pass)¶
| Change | Evidence |
|---|---|
Removed unused storage field from SyncClient and dropped run_startup_sync / HTTP sync-trigger plumbing for that argument | self.storage had no reads; cargo check --all-targets and cargo test --lib (101 tests) pass |
Root .gitignore deduplicated | Same ignore semantics; fewer repeated blocks |
| New engineering docs | code-review-rubric.md, this file |
| Dependencies | cargo machete reported no unused crates |
Explicitly kept: optional columnar-ingest feature tree; Go e2e and compat suites (distinct ownership per rubric Layer D).