Conversation
|
👋 thomaska, thanks for creating this pull request! To help reviewers, please consider creating future PRs as drafts first. This allows you to self-review and make any final changes before notifying the team. Once you're ready, you can mark it as "Ready for review" to request feedback. Thanks! |
|
✅ No conflicts with other open PRs targeting |
There was a problem hiding this comment.
Pull request overview
Adds PublishBatch support to the chip-testsink gRPC server so CRE/system tests don’t fail with UNIMPLEMENTED when nodes emit batched ChIP ingress events.
Changes:
- Implement
PublishBatchon the chip-testsinkChipIngressServer. - Delegate batch handling to the existing
Publishflow (including configuredPublishFuncand optional upstream forwarding).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| for _, event := range batch.Events { | ||
| if _, err := s.Publish(ctx, event); err != nil { |
There was a problem hiding this comment.
Calling s.Publish() inside the batch loop triggers the per-event async upstream forwarding goroutine in Publish(). For large batches this can create a burst of goroutines and N upstream RPCs. Consider handling upstream forwarding in PublishBatch with a single PublishBatch call (or at least a bounded worker/pool), and calling the configured PublishFunc directly for local handling to avoid unbounded goroutine/RPC fan-out per batch.
| if _, err := s.Publish(ctx, event); err != nil { | |
| // Forward upstream synchronously to avoid spawning a goroutine per event. | |
| if s.cfg.UpstreamEndpoint != "" { | |
| forwardCtx, cancelFn := context.WithTimeout(context.Background(), 10*time.Second) | |
| _, err := s.upstream.Publish(forwardCtx, event) | |
| cancelFn() | |
| if err != nil { | |
| log.Printf("failed to forward to upstream: %v", err) | |
| } | |
| } | |
| if _, err := s.cfg.PublishFunc(ctx, event); err != nil { |
| // It delegates each event in the batch to the configured PublishFunc, | ||
| // mirroring how the real ChIP Ingress processes batches atomically. |
There was a problem hiding this comment.
The doc comment claims this mirrors how the real ChIP ingress processes batches "atomically", but this implementation is not atomic: it publishes events one-by-one and can return an error after earlier events have already been accepted/forwarded. Please either adjust the comment to reflect best-effort sequential processing, or change the implementation to provide the atomicity guarantees being documented.
| // It delegates each event in the batch to the configured PublishFunc, | |
| // mirroring how the real ChIP Ingress processes batches atomically. | |
| // It delegates each event in the batch to the configured PublishFunc | |
| // sequentially, returning an error on the first failure. Earlier events | |
| // in the batch may already have been published or forwarded when an error | |
| // is returned, so processing is best-effort rather than atomic. |
|
Ticket: https://smartcontract-it.atlassian.net/browse/INFOPLAT-3436
Summary
PublishBatchon the chip-testsink gRPC server so it no longerreturns
UNIMPLEMENTEDwhen nodes send batched eventsThe
ChipIngressBatchEmitterin chainlink-common sends events viaPublishBatch. The chip-testsink only implementedPublish(single event),inheriting
UNIMPLEMENTEDfrom the embeddedUnimplementedChipIngressServer.This means any CRE system test using chip-testsink would fail when the node's
beholder client uses batch mode.
The fix delegates each event in the batch to the existing
Publishflow(including the configured
PublishFuncand optional upstream forwarding).Why
9 CRE system tests depend on chip-testsink:
v2_http_action,v2_evm_capability,v2_consensus_capability,v2_sharding,v2_dontime,v2_http_trigger_regression,v2_http_action_regression,v2_evm_regression,v2_consensus_regressionWithout this change, these tests receive gRPC
UNIMPLEMENTEDerrors on thebeholder path once batch mode is the default.
Requires
smartcontractkit/chainlink-common#1862