CAMEL-24082: camel-aws-cloudtrail - fix consumer event loss (static cursor, no pagination, window skip)#24726
CAMEL-24082: camel-aws-cloudtrail - fix consumer event loss (static cursor, no pagination, window skip)#24726oscerd wants to merge 1 commit into
Conversation
…ursor, no pagination, window skip) The CloudTrail consumer could silently drop events: - The lookup cursor was a private static field, shared by every cloudtrail consumer in the JVM, so two routes clobbered each other's position. - poll() issued a single non-paginated lookupEvents call and never followed response.nextToken(); combined with the default maxResults=1 only one event per poll was ever retrieved. - The cursor was advanced to the newest event time plus one second, so events arriving within that window (or sharing the newest timestamp) were skipped, with no event-id de-duplication. The cursor is now a per-consumer instance field initialized to the consumer start time (tail from startup rather than replay history). poll() paginates through every page of the window, advances the cursor using the newest event time as an inclusive startTime, and de-duplicates the boundary events by id. The default maxResults is raised to 50 (the AWS maximum); with pagination it is a page-size hint rather than a per-poll cap. Adds CloudtrailConsumerTest (pagination drain + cross-poll boundary de-dup); the module previously had no unit tests, so a test-scoped mockito dependency is added, consistent with the sibling aws2 modules. Documents the behavior and default change in the 4.22 upgrade guide. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Andrea Cosentino <ancosen@gmail.com>
gnodet
left a comment
There was a problem hiding this comment.
Thorough and well-implemented fix for three genuine, long-standing bugs. The diagnosis is accurate, the deduplication logic is sound, and the tests cover the critical scenarios.
What works well:
- Static cursor → instance field: correct fix, verified against the sibling
CloudTrailReloadTriggerTaskpattern. - Pagination loop: properly drains all pages via
nextTokenbefore processing. - Event-id dedup: naturally bounded to events sharing the newest timestamp. The inclusive
startTime+ id-based filtering is a much better approach than the lossy+1000msskip. doStart()guard:if (lastTime == null)correctly means first-start tails from "now", but stop/restart resumes from the prior position — no event gap.
Suggestions (non-blocking):
- The
processBatchimplementation (unchanged by this PR) doesn't setBATCH_INDEX/BATCH_SIZE/BATCH_COMPLETEor callisBatchAllowed(), which is inconsistent with theScheduledBatchPollingConsumercontract (cf.Sqs2Consumer). This is a pre-existing issue, not introduced here, but could be a follow-up improvement. - Consider adding an
@AfterEachteardown in the test to stop the context/component/consumer — prevents leaked threads if more tests are added later. - An empty-poll edge case test would round out the coverage nicely.
Checklist:
- Tests included (pagination drain + boundary dedup)
- Upgrade guide entry documents both behavior changes (maxResults default, startup tail-from-now)
- Generated files regenerated (catalog, DSL builders)
- Backward compat — old default of
maxResults=1was functionally broken; change to 50 is documented - No security concerns
- CI not yet reported
Reviewed with Claude Code on behalf of gnodet. This review was generated by an AI agent and may contain inaccuracies; please verify all suggestions before applying.
davsclaus
left a comment
There was a problem hiding this comment.
Solid fix for three real bugs in the CloudTrail consumer (static cursor, missing pagination, lossy time-window skip). The fix is correct and well-tested:
- Per-consumer instance cursor with
doStart()initialization - Pagination via
nextTokendrains all pages - Bounded dedup by event ID at the boundary timestamp
- Two focused unit tests covering pagination drain and cross-poll boundary dedup
- Behavior changes documented in the 4.22 upgrade guide
- Generated files regenerated
No blocking issues found.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
|
conflict to resolve with upgrade guide |
Description
Fixes CAMEL-24082.
The
camel-aws-cloudtrailconsumer could silently drop CloudTrail events:lastTimewasprivate static Instant, shared by every cloudtrail consumer in the JVM, so two routes (differenteventSource/region) clobbered each other's position, and the value leaked across restarts.poll()issued a singlelookupEvents(...)call and never followedresponse.nextToken(). With the defaultmaxResults = 1, at most one event per poll was retrieved and the rest of the window was discarded.startTime(lastTime.plusMillis(1000)), so events within that 1s window (or sharing the newest timestamp) were skipped forever, with no event-id de-duplication.Fix
lastTimeis now a per-consumer instance field, initialized indoStart()to the consumer start time so it tails from startup instead of replaying history.poll()paginates through every page (nextToken) before delivering.startTime, and boundary events are de-duplicated byeventId(bounded to the newest-timestamp instant) — replacing the lossy+1000msskip.maxResultsraised1 → 50(the AWS maximum). With pagination this is a page-size hint, not a per-poll cap.Tests
The module previously shipped no unit tests. Adds
CloudtrailConsumerTest(with a test-scopedmockito-junit-jupiterdependency, consistent with the siblingaws2-*modules):pollDrainsEveryPage— two pages, asserts all events delivered andlookupEventscalled per page.pollDoesNotRedeliverBoundaryEventsAcrossPolls— asserts the inclusive-startTimeboundary event is not re-delivered on the next poll.Docs
Behavior + default change documented in the 4.22 upgrade guide. Regenerated component/catalog/DSL metadata included.
Backport
Same defect on
camel-4.18.xandcamel-4.14.x; will be backported after merge (fixVersions 4.22.0 / 4.18.4 / 4.14.9). Per project policy the upgrade-guide entry stays onmainonly.Claude Code on behalf of oscerd