CAMEL-20428: camel-kafka - expose the batch-wide topic and partition on the batch exchange - #25006
Conversation
gnodet
left a comment
There was a problem hiding this comment.
Claude Code on behalf of gnodet
✅ LGTM — Well-designed batch header propagation
The "all records must agree" rule is the right design choice — setting a header from the first record would be silently misleading when a batch spans multiple topics or partitions. The implementation is clean and correct:
commonHeaderValue()— simple linear scan, null on first disagreement or missing value. Correct for the expected batch sizes.COMMON_BATCH_HEADERS—List.of(TOPIC, PARTITION)makes it trivially extensible if more headers qualify in the future.- Fully additive — these headers were previously absent from the batch exchange, so zero backward compatibility risk.
- Offset implicitly excluded by not being in
COMMON_BATCH_HEADERS(rather than special-cased in the logic) — clean separation.
Tests: 6 targeted tests covering the key scenarios (uniform, mixed topic, mixed partition, missing header, offset exclusion, empty batch). Testing via the package-private static method without a Kafka broker is the right call for a pure logic function.
Docs: both the component doc (new "Batch Headers" section with example) and upgrade guide entry are well-written and thorough.
JUnit assertions are appropriate here since camel-kafka doesn't have an AssertJ test dependency.
No concerns.
|
merge conflict to resolve |
davsclaus
left a comment
There was a problem hiding this comment.
Clean, well-thought-out PR. The approach is conservative — only propagating headers when all records in the batch agree — and fully additive (previously absent headers are now conditionally set). No existing behavior changes.
Confirmed: No Issues
- Code logic —
commonHeaderValue()correctly handles all edge cases: empty list, single record, uniform values, mixed values, and null values. TheCOMMON_BATCH_HEADERSconstant is an immutableList.of(), thread-safe as a static final field. - Call site placement —
propagateCommonHeaders()is called inprocessBatch()right aftermessage.setBody(exchanges), before processing begins. Both call paths (expired-batch and full-batch) will propagate headers. - Test coverage — 6 unit tests covering uniform topic+partition, mixed topic, mixed partition, null header on one record, offset never propagated, and empty batch.
- Assertion style — JUnit assertions are correct here since AssertJ is not a test dependency in
camel-kafka. - Documentation — New "Batch Headers" section and upgrade guide entry are well-placed and clear.
- Conventions — Branch name, commit format, test visibility, and doc conventions all followed.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code on behalf of Claus Ibsen (@davsclaus).
orpiske
left a comment
There was a problem hiding this comment.
Nice work on this, Andrea — clean implementation and solid test coverage.
Review summary:
- The
commonHeaderValuelogic correctly handles all edge cases (empty batch, uniform values, mixed values, null on any record). - The call site in
processBatch()is correctly placed — headers are set before the route processes the exchange, and there's no interference with thebreakOnFirstErrorhandling (which reads from child exchanges, not the batch message). - The "all-or-nothing" propagation approach is well-designed: setting an arbitrary value (e.g., the first record's) when the batch spans multiple topics/partitions would be misleading.
- Test coverage is thorough: 6 unit tests covering uniform topic+partition, mixed topic, mixed partition, null header, offset never propagated, and empty batch.
One optional suggestion (non-blocking): Recent commits (CAMEL-23789) have been replacing Java constant references with string header names and adding multi-DSL tabs in the component docs. The new "Batch Headers" section uses KafkaConstants.TOPIC in the example and is labeled "Java-only", which is honest — but it could optionally use string header names or add YAML/XML tabs to stay consistent with that direction.
This review does not replace specialized AI review tools (CodeRabbit, Sourcery) or static analysis (SonarCloud).
This review was generated by an AI agent (Claude Code on behalf of orpiske) and may contain inaccuracies. Please verify all suggestions before applying.
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
gnodet
left a comment
There was a problem hiding this comment.
Clean, well-implemented additive feature that propagates common batch metadata headers (topic, partition) onto the batch exchange when all records agree. Good test coverage, proper documentation, and correct backward compatibility.
Notable positives:
- Good design decision to use the "common value" approach — setting a header only when all records agree avoids misleading values when a batch spans multiple topics/partitions
- Tests correctly reside in the same package for unit testing without a Kafka broker
- Upgrade guide entry is clear and notes backward compatibility
- Correctly limits to TOPIC and PARTITION (not OFFSET/TIMESTAMP/KEY which naturally vary per record)
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code on behalf of Guillaume Nodet
|
🧪 CI tested the following changed modules:
🔬 Scalpel shadow comparison — Scalpel: 11 tested, 27 compile-only — current: 9 all testedMaveniverse Scalpel detected 38 affected modules (current approach: 9).
|
…on the batch exchange When using the batching consumer, the exchange carrying the batch only had the record list as its body (plus the manual commit header), so the record metadata was reachable only by inspecting the child exchanges. That makes it awkward to use the topic in the route before the batch is split. Propagate the record metadata headers that every record in the batch agrees on - CamelKafkaTopic and CamelKafkaPartition - onto the batch exchange. A header is only set when all records carry the same value for it: if a batch spans multiple topics or partitions the header is left unset, since no single value would be correct for the batch as a whole. Per-record headers that naturally differ, such as CamelKafkaOffset, are never propagated. The change is additive: these headers were previously absent from the batch exchange, and the headers on the individual record exchanges are unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Andrea Cosentino <ancosen@gmail.com>
0896f20 to
ddb5dba
Compare
gnodet
left a comment
There was a problem hiding this comment.
Re-review: the only new commit (ddb5dba) is a clean rebase resolving the merge conflict that davsclaus flagged. The diff is identical to the previously reviewed and approved code — same 5 files, 219 additions, same tests and documentation. Our prior approval still stands.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code on behalf of Guillaume Nodet
Motivation
CAMEL-20428. With the batching consumer, the exchange carrying the batch had only the record list as its body (plus the manual-commit header). The record metadata was reachable only by digging into the child exchanges, which makes it awkward to use the topic in the route before the batch is split — the issue's example being to store the topic in a variable and reuse it after the split.
Changes
KafkaRecordBatchingProcessornow propagates the record metadata headers that every record in the batch agrees on onto the batch exchange:CamelKafkaTopicandCamelKafkaPartition.The "common" rule is the important part:
CamelKafkaOffset, are therefore never set on the batch. Those stay readable on the individual exchanges in the body.This is additive: these headers were previously absent from the batch exchange, so nothing that worked before changes, and the headers on the individual record exchanges are untouched.
Testing
mvn test -Dtest=KafkaRecordBatchingProcessorCommonHeadersTest: 6/6 pass.mvn clean install -DskipTestsfrom root: success, no stale generated files.Note: the tests use JUnit assertions rather than AssertJ — AssertJ is not a test dependency of
camel-kafka, and the module's existing tests use JUnit assertions throughout, so adding a dependency purely for this test did not seem justified.Docs: new Batch Headers section in the Kafka component docs, plus an upgrade-guide entry. Main-only (4.22.0), no backport.
Claude Code on behalf of Andrea Cosentino (@oscerd).
🤖 Generated with Claude Code