Repository navigation
feat(firehose): honor S3 size and interval buffering hints - #2747
Merged
Merged
Conversation
This was referenced Oct 18, 2026
Buffer records from PutRecord/PutRecordBatch, CloudWatch Logs subscriptions, and SES event destinations in one Firehose-owned engine. Size (decoded bytes) and first-arrival interval hints decide when a batch is written; IntervalInSeconds accepts 0-900 and SizeInMBs 1-128. Detached batches keep their captured configuration and object key, retry in FIFO order with 1-30 s exponential backoff, and are guarded against delete/reset/recreate by runtime stream identities. The server starts one worker, reset discards pending buffers, and graceful shutdown closes admission and makes one final drain. Unsupported delivery settings, explicit destination KMS, S3 backup, and non-S3 destinations reject record admission with HTTP 400 InvalidArgumentException naming the setting, while Create/UpdateDestination and CloudFormation keep accepting every modeled value; an unsupported update pauses already accepted active data. The raw GZIP Logs handoff landed in faiscadev#2720; this adds multi-member and buffered-delivery coverage for it.
As on AWS for DirectPut streams, a batch whose oldest record arrived more than 24 hours earlier is discarded with a warning on its next failed attempt, which also bounds retained memory during prolonged failure. Admission, scheduling, and the final drain now share one delivery-capability check so they cannot drift, the pause diagnostic latches on the stream version instead of copying the configuration on every scan, the final-attempt warning is logged only by the drain, and caller-supplied values are Debug-escaped in warnings.
Prefix and ErrorOutputPrefix allow 1,024 characters, CustomTimeZone 50, RoleARN 1-512, and BucketARN 1-2,048, the Firehose model's length limits, so unbounded values cannot be copied into every buffered batch and object key. Create/UpdateDestination and CloudFormation return InvalidArgumentException as for FileExtension. Also document that constructing the service does not start delivery.
Ref parameters resolve to strings and AWS examples quote numbers, so BufferingHints, RetryOptions.DurationInSeconds, and Enabled flags, including inside S3BackupConfiguration, are coerced before the strict API parser runs. Non-numeric strings still fail the resource.
Spell out that the producer guard is taken before the S3 write lock and both are released before notifications, drop the unguarded options helper that no longer has a production caller, and cover a commit whose account was removed after staging.
Assert the interval lower bound, prove worker liveness in negative waits with healthy control streams, cover SDK buffering-hint and Prefix limits on create and update, and show a Firehose service reset discards pending buffers.
…y trust model The services index now matches the parity row and service page: unsupported settings fail record admission with HTTP 400, bucket-default encryption, replication, and notifications apply, and failed batches are retried for up to 24 hours. The service page also lists destination string limits, CloudFormation coercion, when a malformed bucket ARN is rejected, and that delivery does not evaluate RoleARN or IAM.
…etry every 5s - Capture EncryptionConfiguration.KMSEncryptionConfig in the detached batch's DeliveryConfig and pass it to the guarded S3 writer, so buffered objects are written with SSE-KMS under AWSKMSKeyARN instead of the bucket default. - Retry failed S3 batches every 5 seconds (AWS's documented S3 cadence) up to the 24-hour limit, instead of 1-30s exponential backoff. - Drop remaining KMS admission-rejection docs/tests; ASCII punctuation in docs.
- CloudFormation fails the resource when a modeled boolean/integer/double in the destination block is unparseable or the wrong JSON type (integers are 32-bit, no whitespace trimming), naming the full property path, instead of echoing values DescribeDeliveryStream cannot deserialize; merge overlapping coercion tests. - Test that captured SSE-KMS reaches the S3 write and survives UpdateDestination. - Fix the Firehose parity row ordering.
vieiralucas
force-pushed
the
feat/firehose-s3-buffering
branch
from
October 18, 2026 18:51
6945abc to
cc260a3
Compare
Member
|
Thanks a lot for this, @cavanaug. Real size/interval buffering across producer calls, shared by the Firehose API, Logs subscriptions and SES events, with detach-time config capture, delete/reset/recreate guards, the 24-hour retry limit and a clean shutdown drain, is the piece that makes Firehose behave like the real thing end to end, and it closes out the #2708 plan. The engine design held up well under review. I pushed a few follow-ups before merging, so a couple of points in the PR description no longer apply:
Merged. Really appreciate the whole series. |
Contributor
Author
|
FYI. I didn’t benchmark this but I suspect that fakecloud firehose performance is now vastly improved with the buffering vs previous. |
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
Final increment of the Firehose S3 delivery series tracked in #2708. Stacked on GZIP (#2745) and Xerial Snappy (#2746). Until those merge, this PR's diff against
mainalso contains their commits; the buffering changes are the commits fromfeat(firehose): honor S3 size and interval buffering hintsonward.0 or 60-900check, per Firehose: staged contributions for S3 output fidelity, GZIP/Snappy, and size/time buffering #2708 item 2).DirectPutstreams, stop retrying a batch 24 hours after its oldest record arrived and discard it with a warning; this also bounds memory retained during prolonged destination failure.resetdiscards pending buffers, and graceful shutdown closes admission after the HTTP server stops and performs one final drain with undelivered counts (Firehose: staged contributions for S3 output fidelity, GZIP/Snappy, and size/time buffering #2708 lifecycle guidance).ZIP/HADOOP_SNAPPY, processing other thanAppendDelimiterToRecordalone, format conversion, dynamic partitioning, prefix expressions, non-UTC time zones, explicit destination KMS, S3 backup, and non-S3 destinations — with HTTP 400InvalidArgumentExceptionnaming the setting (Firehose: staged contributions for S3 output fidelity, GZIP/Snappy, and size/time buffering #2708 item 3). Create/UpdateDestination and CloudFormation keep accepting every modeled value; an unsupported update pauses already accepted active data until the configuration is supported again. Admission, scheduling, and the final drain share one capability check.Prefix/ErrorOutputPrefix≤ 1,024,CustomTimeZone≤ 50,RoleARN1–512,BucketARN1–2,048) on create/update and CloudFormation, so unbounded values cannot be copied into every buffered batch and object key. ARN and time-zone patterns are not enforced. Values such as an emptyRoleARN, accepted onmain, now returnInvalidArgumentException.BufferingHints/RetryOptionsintegers andEnabledflags (as produced byRefparameters and AWS's own examples), including insideS3BackupConfiguration.Scope and compatibility
Acknowledgment now means accepted into process-local memory, not immediately visible in S3; poll S3 for delivery. A malformed bucket ARN, which
mainacknowledges and only logs, now fails record admission withInvalidArgumentException(Create/UpdateDestination still accept it). Pending buffers/retries are not durable across a crash; healthy graceful shutdown drains them. Within the 24-hour retention window there is no memory-budget backpressure.Delivery does not assume
RoleARNor evaluate IAM/bucket policies (unchanged frommain; now documented), and Logs/SES may target another account's stream as before. A stream stored without an S3 destination (an S3 configuration missingRoleARN/BucketARN, which AWS rejects) still acknowledges records without writing them, as onmain; documented rather than changed here.The Logs-to-Firehose raw GZIP fix already landed in #2720. The
test(logs)commit only adds multi-member payload and buffered-delivery coverage.Review
Before publication the stack had an independent code/design review (engine concurrency and lifecycle; S3/CloudFormation/Logs/SES integration, tests, and docs) and a STRIDE security review. No Critical findings. Addressed here: stale docs, duplicated readiness checks that could let the drain spin, unbounded
Prefix× retained batches, unlimited retries vs AWS's 24 hours, per-scan configuration copies under global locks, caller-controlled text (object keys, errors, stream names, ARNs) in log lines, now Debug-escaped, the CloudFormation quoted-hint regression, an unused S3 helper, and weak negative E2E waits (now proven against healthy control streams). Not changed (pre-existing, follow-ups): SES's delivery log can list records Firehose refused because the internalFirehoseDeliveryhook returns no result; Firehose is not IAM-enforced.Test plan
Verified on
6945abc71(stacked on Snappyc5907ef3a, upstream774c772d6):cargo check --workspace --all-targets), so the history bisects.cargo test --workspace(every E2E test, including container runtimes) was not rerun on this revision. On the GZIP predecessor it stopped on the unchanged, upstream-reproduced API Gateway fixed-port test (expected 502, got 504 at closed127.0.0.1:54321in this WSL environment). No exclusions were added, and the workspace gate is not claimed as passing.Series context
Refs #2708; completes the four-increment plan. Branch
feat/firehose-s3-bufferingis stacked onfeat/firehose-snappy. As each predecessor merges, I will rebase ontomainso only the buffering commits remain, and revalidate.Summary by cubic
Completes the Firehose S3 delivery series by adding buffered delivery that honors decoded-byte size and first-arrival interval hints from
PutRecord/PutRecordBatch, CloudWatch Logs subscriptions, and SES event destinations, all through one shared engine. Delivery now retries up to 24 hours, rejects unsupported settings with HTTP 400, and enforces destination string limits.New Features:
ZIP, processing, dynamic partitioning, non-S3 destinations) reject record admission withInvalidArgumentException.UpdateDestination.Migration:
BufferingHints/RetryOptionsintegers and fails the resource on unparseable or wrong-typed scalars.Written for commit cc260a3. Summary will update on new commits.