Repository navigation
PublishBatchNoAck client side - #2478
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! |
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 2.
| Benchmark suite | Current: 4fbcc63 | Previous: 7fdf8ba | Ratio |
|---|---|---|---|
BenchmarkKeystore_Sign/nop/in-process |
854.9 ns/op |
410 ns/op |
2.09 |
This comment was automatically generated by workflow using github-action-benchmark.
pkcll
left a comment
pkcll
left a comment
There was a problem hiding this comment.
LGTM overall: additive on the wire, generated code and mocks look right, tests pass locally (go test ./... -race in pkg/chipingress). A few non-blocking suggestions:
1. Make the response contract explicit in the proto comment. The intent is that NoAck only changes when the server responds, not what it responds with. Callers (and the upcoming batch.WithNoAck()) depend on that, since the batch client dispatches per-event callbacks by position and treats a short results slice as RESULTS_MISMATCH. Suggested wording:
// PublishBatchNoAck has the same request/response contract as PublishBatch: same
// validation, partial-delivery and transaction_enabled semantics, same positional
// results and PublishErrorCodes. The only difference is that the server produces to
// Kafka with acks=0 and responds without waiting for broker acknowledgement, so a
// success means validated and handed to the producer, not persisted, and broker-side
// produce failures are not reported. Servers that predate this RPC return UNIMPLEMENTED.
// Use only for loss-tolerant telemetry.It would be good to hold the atlas handler to the same thing: return the full positional results, not an empty PublishResponse.
2. Retry docs. WithRetryPolicy is service-wide, so it now also retries PublishBatchNoAck. docs/retry-policy.md, the WithRetryPolicy godoc and the README "Retries" section list the replayed RPCs; worth adding the new one.
3. Heads-up for the bump. Client embeds pb.ChipIngressClient, so hand-written implementers stop compiling. One I found: noopChipIngressPublisher in chainlink core/services/synchronization/chip_ingress_batch_worker_test.go will need the new method when chainlink bumps pkg/chipingress.
4. batch.WithNoAck() follow-up: #2491. It only selects the RPC in sendBatch, so callbacks and error codes are unchanged, and adds a no_ack attribute to the send metrics only for NoAck clients, so NoAck hand-off latency doesn't skew chip_ingress.batch.request_latency_ms and existing series keep their labels. It depends on point 1: the batch client matches callbacks to results by position.
* chipingress/batch: add WithNoAck option Add batch.WithNoAck(), which makes the batch client send batches with PublishBatchNoAck instead of PublishBatch. The option only selects the RPC: batching, splitting, timeouts, per-event results and error codes are unchanged. A nil callback error means the server validated the event and handed it to its producer, not that it was persisted. Servers that predate PublishBatchNoAck fail the batch with Unimplemented; there is no fallback to PublishBatch. NoAck clients tag send_requests_total, request_latency_ms and config.info with no_ack=true. The attribute is omitted for acked clients so their existing series keep their label set. * chipingress/batch: fix lint in WithNoAck tests Use require for error assertions (testifylint) and drop a redundant PublishErrorCode conversion (unconvert).
INFOPLAT-19548
What
Why
Testing
Notes