feat: implement verifier runner to check received data#13
Open
erenaslandev wants to merge 3 commits into
Open
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds a DuckDB-based verifier container ( ChangesDuckDB Verifier Integration
Sequence Diagram(s)sequenceDiagram
rect rgba(173, 216, 230, 0.5)
Note over Harness: Verifier end-to-end flow
end
participant Harness as cmd/harness
participant Runner as Runner.Run
participant ComposeRunner as ComposeRunner
participant Verifier as verifier container
participant S3 as S3 / LocalStack
participant Disk as /results/verdict.json
Harness->>Runner: Run(tc) with VerifierImage
Runner->>ComposeRunner: NewComposeRunner(RunConfig{VerifierImage})
ComposeRunner->>ComposeRunner: writeCompose (renders verifier service)
Runner->>ComposeRunner: StartService("verifier")
ComposeRunner->>Verifier: docker compose up --profile verify
Verifier->>S3: DuckDB httpfs glob count(*) polling (waitStable)
S3-->>Verifier: row counts during quiet window
Verifier->>S3: DuckDB aggregate query (queryStats)
S3-->>Verifier: total/distinct/null row counts
Verifier->>Disk: write verdict.json (passed/duplicates/errors)
Verifier-->>ComposeRunner: container exits
Runner->>ComposeRunner: WaitForVerifierExit(deadline)
ComposeRunner-->>Runner: exited status
Runner->>Disk: read verdict.json
Disk-->>Runner: ReceiverMetrics
Runner-->>Harness: run result with verdicted metrics
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Makefile (1)
189-189:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winInclude verifier image in
cleanto keep local state consistent.Line 189 removes all bench images except
$(VERIFIER_IMAGE). After this PR,make cleanleaves stale verifier images behind, which can mask Dockerfile/tag changes in local runs.Suggested patch
- docker rmi -f $(GENERATOR_IMAGE) $(RECEIVER_IMAGE) $(COLLECTOR_IMAGE) $(KDC_IMAGE) 2>/dev/null || true + docker rmi -f $(GENERATOR_IMAGE) $(RECEIVER_IMAGE) $(COLLECTOR_IMAGE) $(KDC_IMAGE) $(VERIFIER_IMAGE) 2>/dev/null || true🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Makefile` at line 189, The docker rmi command in the clean target is missing the VERIFIER_IMAGE variable from the list of images to remove. Add the VERIFIER_IMAGE variable to the existing docker rmi command on line 189 alongside GENERATOR_IMAGE, RECEIVER_IMAGE, COLLECTOR_IMAGE, and KDC_IMAGE to ensure all benchmark images are cleaned up and no stale verifier images are left behind.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 78-80: The CI workflow step "Build verifier image" only validates
that the verifier container builds, but does not run unit tests for the verifier
module like it does for the receiver module. Add a new workflow step after the
verifier image build that executes the unit tests for the containers/verifier
module to ensure verifier correctness is validated in PR CI, similar to how
containers/receiver tests are run.
In `@containers/verifier/main.go`:
- Around line 143-153: The row count mismatch check on Line 143 enforces strict
equality (st.total != cfg.Expected) regardless of whether AllowOverDel is
enabled, which causes valid duplicate delivery scenarios to fail. Modify the
condition to allow higher total counts when AllowOverDel is true by checking
against unique count or acceptable loss instead of strict total equality, while
preserving the strict equality check when AllowOverDel is false or not set.
Additionally, update the test case in containers/verifier/main_test.go Lines
36-41 to ensure the Expected field is set to the source-produced count rather
than a higher value that would incorrectly trigger the mismatch check.
In `@internal/config/case.go`:
- Around line 612-631: The validateVerifier function needs to enforce that when
a verifier is configured, there is a deterministic expected count source
available. Add validation to check that tc.Generator.TotalLines is set and
non-zero when tc.Verifier is not nil, since the orchestrator uses
tc.Generator.TotalLines to populate VERIFIER_EXPECTED_LINES. Return an error
with a clear message if the expected lines value is not deterministically set,
to prevent the verifier's exact-count verification from being silently disabled
due to Expected being 0.
In `@internal/orchestrator/docker.go`:
- Around line 367-368: The template correctly omits the singular receiver
service when verifier is enabled (at the {{- else if not .VerifierEnabled }}
condition), but ComposeRunner.populateServiceNames() still pre-populates
receiver metadata unconditionally, causing ReceiverMetricsPort(s) and related
receiver metadata to advertise services that do not exist in verifier mode. Fix
this metadata contract drift by ensuring ComposeRunner.populateServiceNames()
checks the VerifierEnabled flag and skips pre-populating receiver service,
container, and port metadata when the receiver is disabled. Apply the same
conditional check at all sites where receiver metadata is pre-populated
(including the areas around lines 1827-1844 referenced in the comment) to ensure
consistency across the codebase.
In `@internal/runner/runner.go`:
- Around line 2381-2384: The wait duration calculation in runner.go (around
TimeoutDuration and the 30-second buffer) has a logic gap: when
time.Until(runDeadline) returns a non-positive value (deadline has passed or is
now), the wait variable is not updated and retains the full verifier timeout
plus 30 seconds, allowing the run to exceed the hard deadline. Fix this by
ensuring that when remaining time (rem) is zero or negative, wait is explicitly
set to 0 or another appropriate minimal value to respect the hard deadline,
rather than leaving it at the default verifier timeout duration.
- Around line 394-399: The verifier allows overdelivery when
verifier.allow_overdelivery is true, but the later correctness logic still
enforces recvMetrics.LinesReceived <= expectedOut for non-kafka cases,
overriding the verifier's verdict. After the runVerifier call in the
tc.UsesVerifier() block succeeds, you need to track whether overdelivery was
allowed by the verifier, and then modify the subsequent correctness check that
enforces the LinesReceived constraint for non-kafka cases to skip this
validation when the verifier has already allowed overdelivery. Ensure that once
a verifier pass allows overdelivery, the downstream generic non-kafka
over-delivery gate respects that decision and does not flip the result to
failed.
---
Outside diff comments:
In `@Makefile`:
- Line 189: The docker rmi command in the clean target is missing the
VERIFIER_IMAGE variable from the list of images to remove. Add the
VERIFIER_IMAGE variable to the existing docker rmi command on line 189 alongside
GENERATOR_IMAGE, RECEIVER_IMAGE, COLLECTOR_IMAGE, and KDC_IMAGE to ensure all
benchmark images are cleaned up and no stale verifier images are left behind.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0382b102-ebfe-4cac-b7f8-614d096ec18c
📒 Files selected for processing (14)
.github/workflows/ci.yml.goreleaser.ymlMakefilecmd/harness/main.gocontainers/verifier/Dockerfilecontainers/verifier/go.modcontainers/verifier/main.gocontainers/verifier/main_test.gointernal/config/case.gointernal/config/subject.gointernal/orchestrator/docker.gointernal/orchestrator/orchestrator.gointernal/orchestrator/verifier_render_test.gointernal/runner/runner.go
runner correctness gate
Deploying pipebench with
|
| Latest commit: |
9d4f214
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://0c9c8baa.pipebench.pages.dev |
| Branch Preview URL: | https://dt-798.pipebench.pages.dev |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/orchestrator/docker.go`:
- Around line 924-929: The validateVerifier() function currently allows test
cases to configure both verifier and receivers, but populateServiceNames()
intentionally clears receiver metadata for verifier cases while vars.Receivers
still gets populated from the config, causing the template to render
non-existent receiver services. Fix this by adding validation in
validateVerifier() to reject configurations that set both verifier mode and
receivers (check tc.MultiReceiver() for multiple receivers and check for
non-empty singular receiver blocks), or alternatively guard the receiver
template population logic (around lines 1824-1843) to skip it when
tc.UsesVerifier() is true to prevent vars.Receivers from being populated in
verifier mode.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 57a61a7e-d214-4e1d-9408-161c344fbf3e
📒 Files selected for processing (7)
.github/workflows/ci.ymlcontainers/verifier/main.gocontainers/verifier/main_test.gointernal/config/case.gointernal/config/verifier_test.gointernal/orchestrator/docker.gointernal/runner/runner.go
🚧 Files skipped from review as they are similar to previous changes (4)
- containers/verifier/main_test.go
- internal/config/case.go
- internal/runner/runner.go
- containers/verifier/main.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit