refactor: --catchup-delay arg for bench

[shikhar]

No description provided.

[greptile-apps]

Greptile Summary

This PR refactors the bench command's catchup functionality from a binary flag to a configurable delay argument. The --no-catchup boolean flag is replaced with --catchup-delay which accepts a duration (default "20s"), making the delay before the catchup read configurable rather than hardcoded to 20 seconds.

Key changes:

• Replaced --no-catchup flag with --catchup-delay argument accepting duration values
• Updated bench::run() function signature to accept catchup_delay: Duration instead of catchup: bool
• Removed conditional logic that skipped catchup read when no_catchup was true
• Updated integration test to use --catchup-delay 0s instead of --no-catchup

Behavioral implications:

• The catchup read now always executes (previously could be skipped entirely)
• Users can set any delay value including 0 seconds, but cannot skip the catchup phase
• Integration test now runs the full catchup verification instead of skipping it

Confidence Score: 3/5

• The PR introduces a behavioral change where the catchup read now always executes instead of being skippable, which may not align with user expectations and the test change warrants clarification.
• The refactoring is technically sound with proper type handling and consistent API changes across all callsites. However, the behavioral change from "optional catchup read" to "mandatory catchup read with configurable delay" is significant and could affect users who relied on the --no-catchup flag to skip the verification step entirely. The integration test change from skipping the catchup phase to executing it with zero delay masks potential issues and changes test semantics without explanation. The code changes themselves are correct, but the decision to remove the skip option rather than preserve it (e.g., --catchup-delay=None or --skip-catchup flag) deserves clarification.
• tests/integration.rs - The test's behavioral change should be verified to ensure it's intentional and doesn't accidentally enable expensive operations in CI/CD pipelines.

Important Files Changed

Filename	Overview
src/cli.rs	CLI argument refactored from boolean --no-catchup to Duration-based --catchup-delay with default "20s". Improves flexibility by allowing configurable delay before catchup read.
src/main.rs	Updated function call to pass *args.catchup_delay instead of !args.no_catchup. Properly dereferences the humantime::Duration type.
src/bench.rs	Function signature and implementation changed to use catchup_delay: Duration instead of catchup: bool. Behavioral change: catchup read now always executes (previously could be skipped). Uses configurable delay from parameter, making the hardcoded 20s wait now flexible.
tests/integration.rs	Test argument changed from --no-catchup to --catchup-delay 0s. Issue: This changes test behavior - test now runs catchup read immediately instead of skipping it entirely, which may mask issues with the catchup read logic.

Sequence Diagram

sequenceDiagram
    participant CLI as CLI Args
    participant Main as main.rs
    participant Bench as bench::run()
    participant Sleep as tokio::sleep
    participant Read as Catchup Read
    
    CLI->>Main: --catchup-delay 20s (or custom value)
    Main->>Bench: catchup_delay: Duration
    Bench->>Sleep: Sleep for catchup_delay duration
    activate Sleep
    Sleep-->>Bench: (after delay)
    deactivate Sleep
    Bench->>Read: Execute catchup read (always runs)
    activate Read
    Read->>Read: Stream records, calculate metrics
    Read-->>Bench: Catchup sample + hash verification
    deactivate Read
    Bench-->>Main: Result (success or verification error)

Loading

[greptile-apps]

_{4 files reviewed, 1 comment}

_{Edit Code Review Agent Settings | Greptile}