Conversation
Signed-off-by: myetcd <mytech@139.com>
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthroughWarnings for failed chunk-proof and batch-proof status updates now report the error returned by the update operation. ChangesProof status logging
Priority: ⬇️ Low Estimated code review effort: 1 (Trivial) | ~5 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to Both warnings now identify the failed status update. No material merge risk is evident in this narrowly scoped logging change. Architecture SummaryArchitecture risk: 🔵 Low · up to The change affects 1 system. Changed systems: Architecture concerns Review detailsSystems and components
Before / after behavior
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Purpose or design rationale of this PR
Describe your change. Make sure to answer these three questions: What does this PR do? Why does it do it? How does it do it?
What does this PR do? Fixes two
log.Warncalls incoordinator/internal/controller/cron/collect_proof.gothat print the wrong error variable.Why does it do it? In both places the branch is entered because the database update failed,
but the value logged is
checkErr— the error of the readiness check that ran earlier:The
continueabove guaranteescheckErr == nilby the time the update runs, so the warning thatis supposed to describe a failed DB write logs
error=niland the realupdateErris dropped.That is exactly the line an operator needs when chunks/batches never get marked proofs-ready — the
warning fires, but it carries no information about why the write failed. The same copy-paste exists
in
checkBundleAllBatchReady(UpdateBatchProofsStatusByBundleHash).How does it do it? Log
updateErrinstead ofcheckErrin both call sites:No control flow, signature or success-path behaviour changes;
checkErris still used by thepreceding readiness-check warnings, so nothing becomes unused. These are the only two sites of this
kind in the repository — every other
updateErr != nillog incoordinator/,rollup/andbridge-history-api/already logsupdateErr.Both lines date back to the introduction of these functions (
checkBatchAllChunkReadyin #862,checkBundleAllBatchReadyin #1394), where the new update branch was written by copying thepreceding
checkErrbranch.Validation
gofmt -l coordinator/internal/controller/cron/collect_proof.go→ no output.go build ./internal/controller/cron/(incoordinator/) → OK (only the pre-existinggithub.com/rjeczalik/notifycgo deprecation warnings from the macOS SDK).go vet ./internal/controller/cron/→ no findings.github.com/scroll-tech/go-ethereum/log,version as pinned in
coordinator/go.mod), driving the exact control flow with a failing update:WARN ... UpdateChunkProofsStatusByBatchHash failure error=nil hash=0xabcWARN ... UpdateChunkProofsStatusByBatchHash failure error="db: connection refused" hash=0xabcCollectorthat needs a live PostgreSQL/ORM to drive. If a test is wanted here, the cleanestoption is an ORM-level one that asserts the returned error — happy to add it if you prefer.
PR title
Your PR title must follow conventional commits (as we are doing squash merge for each PR), so it must start with one of the following types:
Deployment tag versioning
Has
tagincommon/version.gobeen updated or have you addedbump-versionlabel to this PR?Breaking change label
Does this PR have the
breaking-changelabel?Summary by CodeRabbit