CAMEL-24198: Track Event Hubs checkpoint batching per partition#24903
Conversation
Replace shared checkpoint batch counters and scheduled tasks with per-partition maps so multi-partition consumers commit offsets independently without cross-partition interference.
gnodet
left a comment
There was a problem hiding this comment.
Claude Code review on behalf of gnodet — AI-generated review
CAMEL-24198: Track Event Hubs checkpoint batching per partition
Verdict: APPROVE ✅
Fixes a real concurrency bug — the consumer used shared processedEvents, lastTask, and lastScheduledTask across all partitions, so checkpoint batching from different partitions interfered with each other (e.g., partition 0 reaching batch size could reset the counter that partition 1 was tracking).
What's good
-
Correct data structure choice:
ConcurrentHashMapfor all three per-partition maps (processedEventsByPartition,checkpointTasksByPartition,scheduledTasksByPartition). Even thoughprocessCommit()issynchronized, the maps may be read from other paths, andConcurrentHashMapis the right default for concurrent access. -
Minimal, focused change: Only the
processCommit()method changes — the data structures shift from shared fields to per-partition maps, and the logic is straightforward: look up bypartitionId, create if absent, operate on partition-local state. -
Null-check simplification is correct:
lastTask != null && lastTask.isExpired()→checkpointTask.isExpired(). The null check was needed before becauselastTaskcould be null from a prior invocation. NowcheckpointTaskis guaranteed non-null at theelse ifpoint — either just created (first branch) or fetched from the map (second branch). -
Improved debug logging: All log messages now include the
partitionId, which is essential for diagnosing multi-partition issues in production. -
Good test coverage: 4 tests verify the key partition isolation properties — independent counters, batch-size checkpoint isolation, separate scheduled tasks, and task reuse within the same partition. The
ArgumentCaptorverification that scheduled tasks are distinct instances is a nice touch.
Non-blocking observations
-
Memory cleanup: The per-partition maps grow as partitions are seen but aren't explicitly cleared on
doStop(). This is fine in practice since Event Hub partition counts are bounded (typically 2–32), but aclear()indoStop()would be tidy for completeness. -
Test assertions style: The tests use JUnit 5
assertEquals/assertNotSamerather than AssertJ. Not a blocker — both are valid in Camel, and the assertions are clear.
Clean fix for a real partition-interference bug. 👏
…ching Clear per-partition checkpoint maps in doStop(), switch tests to AssertJ, and add coverage for stop-time cleanup.
|
Last change summary — commit What changedReview feedback on the per-partition checkpoint batching work: 1.
2.
3.
Verification
Branch history
|
gnodet
left a comment
There was a problem hiding this comment.
Re-review of commit 2 (0905a93) — addresses review feedback
This commit cleanly addresses both non-blocking observations from my initial review:
-
Memory cleanup in
doStop()✅ — All three per-partition maps (processedEventsByPartition,scheduledTasksByPartition,checkpointTasksByPartition) are now cleared indoStop(), placed correctly after stopping theprocessorClientbut beforesuper.doStop(). This prevents stale state from accumulating across restart cycles. -
AssertJ assertion style ✅ — All assertions migrated from JUnit
assertEquals/assertNotSameto idiomatic AssertJ (assertThat(...).isEqualTo(...),.isZero(),.hasSize(),.isNotSameAs(),.isEmpty()). Theassertj-coretest dependency was added topom.xml. -
New test
doStopClearsPerPartitionCheckpointState()✅ — Good addition that verifies all three maps are empty afterdoStop()and confirms the processor client is stopped. The test properly sets up theprocessorClientmock via reflection and populates two partitions before stopping.
Approval from the previous review stands. Well done addressing the feedback!
Claude Code on behalf of gnodet — AI-generated review
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
|
🧪 CI tested the following changed modules:
🔬 Scalpel shadow comparison — Scalpel: 9 tested, 29 compile-only — current: 9 all testedMaveniverse Scalpel detected 38 affected modules (current approach: 9).
|
CAMEL-24198 is implemented, tested, committed, and pushed.
Issue
CAMEL-24198 — camel-azure-eventhubs - Redesign checkpoint store to use per-partition batching
The consumer used a single shared
processedEvents,lastTask, andlastScheduledTaskacross all partitions, so checkpoint commits from different partitions could interfere with each other.Fix
In
EventHubsConsumer.java, replaced shared fields with per-partition maps:processedEventsByPartitioncheckpointTasksByPartitionscheduledTasksByPartitionEach partition now tracks its own batch count and schedules its own timeout checkpoint independently.
Tests
Added
EventHubsConsumerPerPartitionCheckpointTest.javawith 4 tests:12/12 tests passed (including existing shutdown and updater task tests).
Git
CAMEL-24198-eventhubs-per-partition-checkpoint-batching8896d7755582origin