Dual Storage Architecture

[zaibkhan]

Mirrors ai-code-review-evaluation#6 for like-for-like benchmarking.

• Base: dual-storage-baseline
• Head: dual-storage-enhanced

Original PR excerpt:

Test 6

[codoki-pr-intelligence]

Codoki PR Review

Summary: Preserve update consistency, fix metrics/log context
What’s good: Mode 3 now reads from unified storage and writes asynchronously to legacy with timeouts; List is implemented and metrics/logging are added throughout.
Review Status: ❌ Requires changes
Overall Priority: High

Issues (Critical & High only)

Severity	Issue	Why it matters
High	Correctness — Legacy update may diverge from storage; wrong metrics rec...	…/rest/dualwriter_mode3.go Two issues in this Update path: (1) the error path records legacy metrics for a storage failure; (2) the legacy update reuses objInfo instead of the unified-store result, risking divergence between storages (e.g., different defaulting/resourceVersion). Wrap objInfo so legacy gets the exact object returned by storage, and fix the metrics recorder.

Showing top 1 issues. Critical: 0, High: 1. See inline suggestions for more.

Key Feedback (click to expand)

• Needs improvement: Update no longer propagates the unified-store result to legacy, risking divergence; a few metrics calls use the wrong recorder/labels and one log context loses fields.
• Testing: Consider adding a test that asserts Update writes the exact object returned by storage to legacy (e.g., wrap objInfo so legacy receives the unified-store result). Also add an assertion that metrics use kind (not name) for labels and that storage vs. legacy recorders are correctly selected.
• Open questions: Should legacy writes proceed even if the request context is canceled? Today the goroutines inherit the request context and may abort immediately; if best-effort writes are desired, consider decoupling from the parent context.

Confidence: 3/5 — Needs work before merge (1 high · status: Requires changes)

Sequence Diagram

sequenceDiagram
    participant Caller
    participant DualWriterMode3
    participant Storage
    participant Legacy
    Caller->>DualWriterMode3: Update(name, objInfo,...)
    DualWriterMode3->>Storage: Update(...)
    alt storage error
        Storage-->>DualWriterMode3: err
        DualWriterMode3-->>Caller: error
    else success
        Storage-->>DualWriterMode3: res, async, nil
        DualWriterMode3->>Legacy: Update(..., async)
        Legacy-->>DualWriterMode3: (ignored in async)
        DualWriterMode3-->>Caller: res, async, nil
    end

Loading

React with 👍 or 👎 if you found this review useful.

[codoki-pr-intelligence]

⚠️ High: Two issues in this Update path: (1) the error path records legacy metrics for a storage failure; (2) the legacy update reuses objInfo instead of the unified-store result, risking divergence between storages (e.g., different defaulting/resourceVersion). Wrap objInfo so legacy gets the exact object returned by storage, and fix the metrics recorder.

Suggested change

	defer cancel()
	```go
	if err != nil {
	log.Error(err, "unable to update in storage")
	d.recordStorageDuration(true, mode3Str, options.Kind, method, startStorage)
	return res, async, err
	}
	// Ensure legacy receives the same object returned by Storage to avoid divergence
	_, _, errObjectSt := d.Legacy.Update(ctx, name, &updateWrapper{upstream: objInfo, updated: res}, createValidation, updateValidation, forceAllowCreate, options)

[codoki-pr-intelligence]

🔷 Medium: This loses the per-call fields (name/kind/method) by attaching the base logger instead of the enriched one; also the storage metrics label uses name instead of kind. Attach the enriched logger and use options.Kind for metrics consistency.

go
ctx = klog.NewContext(ctx, log)
// ...
d.recordStorageDuration(false, mode3Str, options.Kind, method, startStorage)

[codoki-pr-intelligence]

🔷 Medium: This records legacy DeleteCollection timing using the storage metrics function; use the legacy recorder for correct attribution.

Suggested change

	_, err := d.Legacy.DeleteCollection(ctx, deleteValidation, options, listOptions)
	```go
	d.recordLegacyDuration(err != nil, mode3Str, options.Kind, method, startLegacy)