feat(aws_kinesis): aggregate records, stop dropping partial failures - #45
Draft
smolaon wants to merge 4 commits into
Draft
feat(aws_kinesis): aggregate records, stop dropping partial failures#45smolaon wants to merge 4 commits into
smolaon wants to merge 4 commits into
Conversation
added 3 commits
August 31, 2026 19:02
PutRecords and PutRecordBatch return HTTP 200 with per-record errors, but event_status was Delivered unconditionally. With acknowledgements enabled a source deleted its message for records that were never written, and request_retry_partial defaults to off so they were not retried either.
serde_json::to_writer_pretty emits literal newlines, which would split one event into several unparseable fragments on the consumer. The guard now reads the serializer config rather than the built serializer so it can see the flag.
smolaon
force-pushed
the
feat/kinesis-aggregation
branch
from
September 1, 2026 10:04
eab3fd8 to
aece0b7
Compare
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
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
PutRecordsreturns HTTP 200 with per-record errors, butevent_statusreportedDeliveredunconditionally, so a source with acknowledgements enabled deleted its message for records that were never written.request_retry_partialdefaults to off, so they were not retried either. Applies to firehose too, which shares the response type.Tests:
cargo check --testsclean across streams + firehose + the integration-test feature (src/lib.rshas#![deny(warnings)]),cargo test aws_kinesis9/9 passed, plus two end-to-end probes against a locally built binary.Screenshots: n/a -- no visible change.
Details
A consumer must decompress and then split on newlines, and the rollout order is not symmetric.
An unaggregated record is a single-line payload, so a splitting consumer handles both formats and a
stream can carry both during a rollout. The reverse is not true: an old consumer receiving a
multi-line record fails to parse it, and depending on its error handling that can stall a shard
rather than skip a record. Deploy the consumer change everywhere before enabling this on any
producer.
Errored, notRejected, is the correct status for a partial failure -- and the difference isload-bearing. A per-record
ProvisionedThroughputExceededExceptionis transient, and onlyErroredmaps to
BatchStatus::Errored, which theaws_s3source turns intoProcessingError::ErrorAcknowledgementand therefore leaves its SQS message in the queue.Rejectedwould be deleted instead:delete_failed_messagedefaults totrue(
src/sources/aws_s3/sqs.rs), so nacking asRejectedwould have preserved the bug.request_retry_partialis deliberately left defaulting to off here. Turning it on is the rightoperational setting, but it is shared with firehose, whose
should_retry_responseretries thewhole request rather than the failed indices -- so flipping the shared default would start
duplicating successfully-written firehose records. Set it per sink in the config instead; the status
fix is the backstop that makes the default safe either way.
encoding.json.prettyis rejected under aggregation, not just non-JSON codecs. Newline framingis only sound if no event can contain a literal newline, and compact JSON escapes them as the two
characters
\n.to_writer_prettyemits real ones, which would split one event into severalunparseable fragments. The guard reads
EncodingConfig::config()rather than the builtSerializer,because the built form does not expose the flag. It is scoped to aggregation, so
prettywithaggregation off still builds.
partition_key_fieldis ignored when aggregation is on, with awarn!rather than an error(
streams/config.rs). Events in one record may disagree on the field, so a random UUID is generatedper record, which keeps the MD5 hash distribution -- and therefore shard distribution -- uniform. A
warning rather than a hard failure because the option has no correctness impact here, and erroring
would crash-loop a sink on config that was previously valid.
aggregation.max_bytesbounds the uncompressed input (default 256 KiB, capped at 900,000).The compressed size is not known until the batch closes and
RequestBuilderemits exactly onerequest per batch, so staying well under the 1 MB record limit means even wholly incompressible
input still fits.
Its own config table, not
batch.batchis hard-capped to thePutRecordslimits and itsunits are records per API call; these are events per record. Reusing it would silently reinterpret
500 / 5 MB.
Implementation Plan
Format: NDJSON inside the existing zstd frame, not KPL. The wire format stays "a zstd frame", so
the consumer's codec detection needs no change -- it decompresses as today and then splits on
newlines. KPL would need protobuf plus an MD5 trailer, and on the Go side
awslabs/kinesis-aggregationfans out across ~75go.modfiles. NDJSON needs zero new deps oneither side, and we control both ends -- there is no interop requirement. It also reuses upstream
machinery wholesale:
aws_s3already does batch -> newline framing -> compress once.Shape:
The existing builder is
RequestBuilder<KinesisProcessedEvent>withtype Events = Event, andRequestBuilder::encode_eventsconstructs a freshCompressorper call -- which is exactly whyevery record is its own frame today. Taking
Vec<Event>is the whole mechanism; it reuses theupstream
impl Encoder<Vec<Event>> for (Transformer, Encoder<Framer>)thataws_s3relies on. Notethe encoder type changes too: the existing builder uses the unframed
Encoder<()>, which has noframer and hence no newline suffix. That is why a separate builder is cleaner than mutating the
shared one.
Everything downstream of the request builder is untouched: one aggregate is one
KinesisRequest, sothe index-based partial-failure retry in
KinesisRetryLogickeeps working, and finalizers are mergedvia
Vec::take_finalizersso an ack or nack applies to every event in the record. Acknowledgementgranularity is unchanged by aggregation, incidentally -- the driver already takes finalizers off the
whole
BatchKinesisRequestbefore the call and stamps one status across all of them, so up to 500records always shared one ack.
streams/aggregation/config.rsKinesisAggregationConfig,BatcherSettingsbuilt directly (not viaBatchConfig, which is hard-capped to PutRecords limits)streams/aggregation/request_builder.rsEvents = Vec<Event>,Encoder<Framer>, merged finalizers, one UUID key per recordstreams/aggregation/sink.rsstreams/aggregation/build.rsstreams/config.rs,streams/mod.rs,request_builder.rsaggregationfield, build branch,KinesisRequest::newsometadatastays privateservice.rsevent_statusreportsErroredwhenfailure_count > 0, with unit testsNot changed:
record.rs(an aggregate is just a biggerBytes), the baseconfig.rs,lib/codecs/*,Cargo.toml. Zero new crates.Why not a codec-level
BatchSerializer: that abstraction is for serializers producing one opaqueblob with no per-event framing (parquet, arrow). NDJSON is per-event serialize plus a framer, which
Encoder<Framer>already is. Going throughlib/codecswould cross a crate boundary and mutate theglobal serializer enum for every sink.
Measured, on 400 real CloudTrail events (3,680 B/event raw):
Diminishing returns justify the 50-ish default: a larger record buys single digits while
concentrating bytes against the 1 MB/s/shard write cap.
Verification. End to end against a locally built binary and a fake Kinesis endpoint, 60 events
in:
Event bytes are 29,657 in both cases, and the events are byte-identical in frame order once the
stdinsource's per-runhostandtimestampare excluded. The decoded delta is exactly +60 for 60events -- pure newline framing, no content change -- which confirms the encoding transformer still
applies per event, as it must.
A second probe drives the serializer guard through six configs: compact
jsonandnative_jsonbuild;
jsonwithpretty: true,text, andraw_messageare refused with the reason named; andprettywith aggregation off still builds, confirming the guard cannot break an existing config.