Skip to content

feat(cost): estimate disk and object-store operation counts - #354

Merged
zzylol merged 5 commits into
mainfrom
feat/issue-345-storage-operations
Sep 8, 2026
Merged

zzylol merged 5 commits into
mainfrom
feat/issue-345-storage-operations

Conversation

@zzylol

@zzylol zzylol commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

Why

Equal scan bytes can require different storage request counts and costs. Addresses #345 independently of #349 and #346.

What and how

Estimate disk reads/writes and object GETs/PUTs as sum(ceil(extent_bytes / bytes_per_request)) × executions. Count shared physical nodes once. Require fresh, complete node/statistics evidence; scan read extents must match authoritative source-read bytes. Add separately calibrated request costs to ranking and export totals, per-node terms, and provenance.

Before this PR

A shared scan of two 40-byte objects over three evaluations reports 240 scan bytes but no GET estimate.

After this PR

With 32-byte requests it reports 12 GETs; a build-once scan reports four. A 160-byte write with 64-byte requests adds three writes per execution. JSON-to-ranking/export coverage verifies 160 raw GETs versus 10 candidate GETs, then explicit writes that reverse the preference. A request-only objective works with zero base CPU/byte coefficients.

Verification

  • Independent review completed; added request-only objective support with a failing-before/passing-after regression.
  • Passed cargo test --workspace --locked, including current main's test(promql): verify TopK planning and numeric reference results #351 tests.
  • Passed cargo clippy --workspace --all-targets --all-features --locked -- -D warnings, formatting, and diff checks.
  • Tests cover per-extent rounding, monotonicity, integer overflow, shared reads, explicit writes, retained-state multiplicity, stale/missing/rebound evidence, strict JSON, ranking/export, and rejection of absent/invalid/all-zero objectives.

The examples are executable analytical checks, not measured I/O. Screenshots and empirical runtime benchmarks: not applicable; none were produced.

Architectural decisions

An optional deployment-snapshot profile binds request sizing and storage accesses to complete physical node/statistics records. Counts remain separate from bytes until calibration. Missing profiles preserve the existing objective without fabricating request counts.

Limitations and follow-up

The payload model excludes metadata, retries, speculative prefetch, multipart control requests, and empty-object creation. Lifecycle-specific maintenance and cache integration remain separate. Rust snapshot constructors gain an optional profile field; existing JSON remains supported. This branch includes current main and is not stacked on the other cost PRs.

Human review — do not complete with an agent

  • The MVP boundary is correct.
  • New conceptual layers or public interfaces are necessary.
  • The before/after description matches the intended product behavior.
  • Human reviewer:
  • Decision and rationale:

@zzylol

zzylol commented Sep 7, 2026

Copy link
Copy Markdown
Contributor Author

@Selvomega

@zzylol
zzylol marked this pull request as ready for review September 7, 2026 16:45
@zzylol
zzylol requested a review from Selvomega September 7, 2026 16:54
@zzylol

zzylol commented Sep 8, 2026

Copy link
Copy Markdown
Contributor Author

Review and fixes pushed in c3c5846 (plus main merge d273191, including #357 shared resource compatibility). Finding P2: an empty/whitespace base calibration version was accepted, including storage-only objectives. The constructor now rejects missing provenance before snapshot lookup; the new regression was observed failing before the fix and passing afterwards. Additional passing tests cover zero extents/all four request kinds, multiplicity and cross-node overflow, and node/source/statistics rebinding. Those cases confirmed the original storage arithmetic rather than identifying additional bugs. Validation passed: cargo test --workspace --locked; cargo clippy --workspace --all-targets --all-features --locked -- -D warnings; format/diff checks (363 mapping unit tests and 7 storage integration tests included). Normal push only; no PR merge or human-review attestation.

@zzylol

zzylol commented Sep 8, 2026

Copy link
Copy Markdown
Contributor Author

Updated in e44e0f2: truly rebased onto main c3410d1 with linear PR history and all previous correctness fixes preserved. StorageResources now has one canonical definition in asap_types::resources, covering disk reads/writes and object-store GET/PUT counts; the mapping module re-exports the shared type for import compatibility. Estimation, evidence binding, and calibration remain in mapping. Added serialization, per-dimension checked-add overflow, shared-type identity, and missing-profile regressions. Full workspace tests --locked and strict Clippy (workspace, all targets/features) pass, including 364 mapping unit tests, 8 storage integration tests, and 165 types tests. Updated with an explicit force-with-lease; not merged.

@zzylol
zzylol force-pushed the feat/issue-345-storage-operations branch from c3c5846 to e44e0f2 Compare September 8, 2026 19:12
@zzylol

zzylol commented Sep 8, 2026

Copy link
Copy Markdown
Contributor Author

Follow-up: split the remaining shared definitions into resources/cpu.rs (ModeledCpu and MeasuredCpu), resources/measurement.rs (Measurement), and resources/physical.rs (PhysicalResources and MeasuredResources). resources.rs retains module declarations, re-exports, and cross-resource contract tests. Existing root import paths, fields, units, defaults, and JSON formats are unchanged. Full workspace tests --locked and strict workspace Clippy across all targets/features pass.

@zzylol
zzylol force-pushed the feat/issue-345-storage-operations branch from 1cd7350 to 160d23a Compare September 8, 2026 19:30
@zzylol

zzylol commented Sep 8, 2026

Copy link
Copy Markdown
Contributor Author

Rebased onto merged #353 (main 53e76d4), retaining cache and storage evidence fields and both sets of exported cost annotations. A new regression exposed an unsupported combination: aggregate cache hit ratios cannot determine independently rounded per-extent storage requests. Comparison now fails closed for CacheProfile::Evidence plus storage_io until cache-aware extent evidence exists; explicit NoCache plus storage remains supported. The regression failed before the guard and passes afterward. Full local validation and CI are being run before merge.

@zzylol
zzylol merged commit e6fb07f into main Sep 8, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant