Conversation
gnodet-bot
left a comment
There was a problem hiding this comment.
Clean metadata-only change — adds by_duration to the autoOffsetReset enum list and updates the Javadoc and catalog descriptor with clear documentation of the format and Kafka 4.0 requirement. The @UriParam(enums=...) attribute is tooling/docs only (no runtime validation), and the value is passed through to the Kafka client as-is, so this is safe.
No functional change, no new code paths, no test needed.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
oscerd
left a comment
There was a problem hiding this comment.
Thanks for this — the change itself is correct and low-risk. Camel passes autoOffsetReset straight through to the Kafka ConsumerConfig, so by_duration:PT5M already reaches the client; adding by_duration to the @UriParam(enums=...) and documenting the by_duration:<ISO8601> syntax is the right way to surface it for tooling and docs (listing the base by_duration as the enum hint while the description carries the full form is fine).
One thing to fix before it can go green: the generated artifacts are only partially updated. The diff has the component's own kafka.json and KafkaConfiguration.java, but an @UriParam enum/description change also regenerates:
- the catalog mirror
catalog/camel-catalog/src/generated/resources/org/apache/camel/catalog/components/kafka.json(I checked — it still lists onlylatest,earliest,noneonmain), and - the component docs
components/camel-kafka/src/main/docs/kafka-component.adoc(and its catalog mirror undercatalog/.../docs/), since the option's description changed.
CI's "uncommitted changes" check regenerates everything from the full reactor and fails if the tree is dirty, so those files need to be regenerated and committed. Easiest is a full-reactor build from the repo root (mvn clean install -DskipTests, or at least build components/camel-kafka then the catalog/docs modules) and commit whatever it changes. Once the catalog mirror and docs carry by_duration too, this should be good.
This review was generated with AI assistance and reviewed/issued by the human operator. Claude Code on behalf of oscerd
davsclaus
left a comment
There was a problem hiding this comment.
Thanks! Supporting by_duration is useful, but the enum approach doesn't work:
by_durationon its own is not a valid value. The Kafka client needsby_duration:<ISO-8601 duration>, e.g.by_duration:PT5M. A bareby_durationfails with "<:duration>part is missing in by_duration auto offset reset strategy". The Camel catalog compares a value exactly (ignoring case) against theenumslist. So after this change tooling suggestsby_duration, which Kafka rejects, while the realby_duration:PT5Mstill fails catalog, IDE andcamel validatechecks. Camel already passes the value straight through to the client, soby_duration:PT5Mworks at runtime today. Suggest removingenumsfromautoOffsetResetand listing the values in the description:latest,earliest,none,by_duration:<ISO-8601 duration>.- Generated files are missing. Only the component's own
kafka.jsonwas updated. Please regenerate and commit the camel-catalogkafka.json,KafkaEndpointBuilderFactory(endpoint-dsl) andKafkaComponentBuilderFactory(component-dsl), or the CI uncommitted-changes check will fail. - Description text: the new sentence runs straight into the old one ("...throw exception to the consumer by_duration:..."). The existing text is also stale: it mentions ZooKeeper, and it says
failwhere the value is actuallynone. A good chance to clean it up. - Test: please add a small test, e.g. a catalog
validateEndpointPropertiescheck thatkafka:t?autoOffsetReset=by_duration:PT5Mis accepted, and/or one thatcreateConsumerProperties()passes the value through. - JIRA: please create a CAMEL JIRA issue and put its key in the PR title and commit message.
Claude Code on behalf of davsclaus
eda07a3 to
123c301
Compare
d7705cf to
44125f1
Compare
gnodet-bot
left a comment
There was a problem hiding this comment.
Re-review after d7705cf — checking whether the feedback from @davsclaus and @oscerd has been addressed.
Previous findings status
| # | Finding (davsclaus) | Status |
|---|---|---|
| 1 | enums attribute doesn't work for parameterised by_duration:<ISO-8601> — remove it |
✅ Addressed — enums removed from @UriParam, type changed from "enum" to "string" in JSON, "enum" array removed |
| 2 | Generated files missing (catalog JSON, endpoint-DSL, component-DSL) | ✅ Addressed — all 4 generated files now in the diff |
| 3 | Description stale (ZooKeeper reference, fail → none) |
✅ Addressed — description completely rewritten, accurate and clear |
| 4 | Add a test | ✅ Addressed — byDurationAutoOffsetResetIsPassedThroughToConsumerConfig added |
| 5 | Create JIRA issue, key in PR title | ✅ Addressed — CAMEL-25091 in title |
| # | Finding (oscerd) | Status |
|---|---|---|
| 1 | Catalog mirror and docs need regeneration | ✅ Addressed — catalog kafka.json and DSL factories updated |
All previous findings are addressed. ✅
New finding
kafka.json files. Both catalog/…/kafka.json and components/…/kafka.json have been reformatted from the project's compact single-line-per-property format to multi-line pretty-printed JSON. On main, each property entry is a single line like:
"autoOffsetReset": { "index": 13, "kind": "property", … }The PR expands every property to ~15 lines, turning a 2-property semantic change into an 8,800-line diff. This:
- Makes the JSON portions unreviewable (the actual
autoOffsetResetchange is buried in reformatting noise) - Will conflict with any other PR touching
kafka.json - Doesn't match the project's generated-file format
The fix is to regenerate these files using the standard Camel build (e.g. mvn generate-resources -pl components/camel-kafka then mvn generate-resources -pl catalog/camel-catalog, or a full mvn install -DskipTests), which produces compact output matching main. The current files appear to have been hand-edited or formatted with a different JSON serializer.
Everything else looks solid — the KafkaConfiguration.java changes, the updated Javadoc, the DSL factories, and the test are all correct.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
gnodet-bot
left a comment
There was a problem hiding this comment.
Re-review after 44125f1 — all previous findings addressed.
| # | Finding | Status |
|---|---|---|
| 1 | enums attribute removed (davsclaus) |
✅ Fixed — enums removed from @UriParam, type → "string" in JSON, "enum" array removed |
| 2 | Generated files missing (davsclaus, oscerd) | ✅ Fixed — catalog JSON, endpoint-DSL and component-DSL factories all updated |
| 3 | Stale description: ZooKeeper ref, fail → none (davsclaus) |
✅ Fixed — description completely rewritten, accurate |
| 4 | Add a test (davsclaus) | ✅ Fixed — byDurationAutoOffsetResetIsPassedThroughToConsumerConfig added |
| 5 | Create JIRA issue (davsclaus) | ✅ Fixed — CAMEL-25091 in title |
| 6 | JSON reformatting / 8,800-line diff (gnodet-bot) | ✅ Fixed — compact single-line format matching main, diff is now 152 lines |
Clean metadata-only change. No functional change, no new code paths. The autoOffsetReset value is passed through to the Kafka client as-is (already was), so adding by_duration documentation and removing the restrictive enums constraint is the right approach.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
44125f1 to
71b6fc6
Compare
gnodet-bot
left a comment
There was a problem hiding this comment.
Re-review after 71b6fc6 (squash/rebase of previous commits into one clean commit).
Previous findings status
All 6 findings from @davsclaus and @oscerd remain addressed — the diff is semantically identical to the previously approved 44125f1.
@Croway's feedback addressed
@Croway noted the previous test was essentially testing a getter/setter. The test was replaced with autoOffsetResetHasNoEnumConstraint — a reflection-based check that the @UriParam annotation carries no enums constraint. This directly verifies the invariant that matters (the enum restriction was removed) rather than testing passthrough behavior. Good change.
Clean metadata-only PR. No functional change, no new code paths.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
71b6fc6 to
a772a76
Compare
davsclaus
left a comment
There was a problem hiding this comment.
Thanks for the updates. Removing the enum, the description fix (none, by_duration:<ISO-8601>) and the JIRA linkage all look good.
One thing left: the two DSL factories look hand-edited rather than regenerated. The generator writes the Javadoc blank lines as * (with a trailing space), and the PR turns the existing ones into * in KafkaComponentBuilderFactory and KafkaEndpointBuilderFactory. A real regeneration puts them back, so CI's uncommitted-changes check will fail. Please build components/camel-kafka and then catalog, dsl/camel-endpointdsl and dsl/camel-componentdsl, and commit the output.
Optional: a catalog validateEndpointProperties("kafka:t?autoOffsetReset=by_duration:PT5M") test would check what users actually hit, rather than the annotation via reflection.
Claude Code on behalf of davsclaus
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
a772a76 to
be8c608
Compare
gnodet-bot
left a comment
There was a problem hiding this comment.
Re-review after be8c608 — checking whether @davsclaus's latest feedback has been addressed.
Previous findings status
| # | Finding (davsclaus) | Status |
|---|---|---|
| 1 | DSL factories hand-edited — blank Javadoc lines use * instead of * (trailing space), CI uncommitted-changes check would fail |
✅ Addressed — blank Javadoc lines now use * (trailing space) matching the generator's output on main |
| 2 | (Optional) catalog validateEndpointProperties test instead of reflection-based test |
⏭️ Not done — test remains autoOffsetResetHasNoEnumConstraint (reflection-based). This was optional, and the current test is valid: it directly verifies the invariant that the enums constraint was removed |
All mandatory findings from all reviewers are addressed. The diff is 139 lines, metadata-only (description + type change from enum to string, enums attribute removal). No functional change, no new code paths. DSL factories now appear properly regenerated.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
be8c608 to
c0da6e8
Compare
gnodet-bot
left a comment
There was a problem hiding this comment.
Re-review after c0da6e8 — squash/rebase of previous commits into one clean commit.
Previous findings status
All 6 mandatory findings from @davsclaus and @oscerd remain addressed (identical to previously approved be8c608).
What changed
The previously-optional suggestion from @davsclaus was adopted: the reflection-based autoOffsetResetHasNoEnumConstraint test was replaced with byDurationAutoOffsetResetPassesCatalogValidation — a catalog validateEndpointProperties test that verifies by_duration:PT5M is accepted end-to-end. camel-catalog was added as a test dependency to enable this. Better test — validates what users actually hit rather than the annotation's internal state.
Clean metadata-only PR. No functional change, no new code paths.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
gnodet-bot
left a comment
There was a problem hiding this comment.
Re-review after c0da6e8 (squash/rebase incorporating @davsclaus's optional suggestion).
Previous findings status
All 6 mandatory findings from @davsclaus and @oscerd remain addressed — the diff is semantically identical to the previously approved be8c608, with one improvement:
| # | Finding | Status |
|---|---|---|
| 1 | enums attribute removed (davsclaus) |
✅ |
| 2 | Generated files regenerated (davsclaus, oscerd) | ✅ |
| 3 | Stale description fixed (davsclaus) | ✅ |
| 4 | Test added (davsclaus) | ✅ Upgraded — see below |
| 5 | JIRA issue created (davsclaus) | ✅ |
| 6 | DSL trailing spaces fixed (davsclaus) | ✅ |
Delta since be8c608
@davsclaus's optional suggestion adopted: reflection-based autoOffsetResetHasNoEnumConstraint replaced with byDurationAutoOffsetResetPassesCatalogValidation — validates kafka:topic?autoOffsetReset=by_duration:PT5M through CamelCatalog.validateEndpointProperties, which is what end users actually hit. camel-catalog test dependency added correctly.
Clean metadata-only PR. No functional change, no new code paths.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
501d7df to
eda5be7
Compare
gnodet-bot
left a comment
There was a problem hiding this comment.
Re-review after eda5be7 — rebase onto current main with @davsclaus's latest suggestions applied.
Previous findings status
All findings from @davsclaus, @oscerd, and @Croway remain addressed.
| # | Finding | Status |
|---|---|---|
| 1 | enums attribute removed (davsclaus) |
✅ |
| 2 | Generated files regenerated (davsclaus, oscerd) | ✅ |
| 3 | Stale description fixed (davsclaus) | ✅ |
| 4 | Test upgraded to catalog validation (davsclaus) | ✅ |
| 5 | JIRA issue created (davsclaus) | ✅ |
| 6 | DSL trailing spaces fixed (davsclaus) | ✅ |
| 7 | DefaultCamelContext closed via try-with-resources (davsclaus) |
✅ |
| 8 | topic added to validateProperties map — required after CAMEL-25118 (davsclaus) |
✅ |
| 9 | <ISO-8601> angle brackets replaced with PnDTnHnM — avoids double-escaping in DSL Javadoc (davsclaus, optional) |
✅ |
Clean metadata-only PR. No functional change, no new code paths.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
gnodet-bot
left a comment
There was a problem hiding this comment.
Re-review after 6c54c95 — regeneration commit (line-wrap adjustment in generated DSL Javadoc).
Delta since eda5be7: 2 files, 4 lines each — KafkaComponentBuilderFactory.java and KafkaEndpointBuilderFactory.java have a Javadoc line re-wrapped from requires Kafka 4.0 or\n later). to requires\n Kafka 4.0 or later). This is the output of the project's regen script. No semantic change.
All 9 mandatory findings from @davsclaus, @oscerd, and @Croway remain addressed.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
davsclaus
left a comment
There was a problem hiding this comment.
LGTM. All earlier comments are addressed, Kafka 4.3.1 on main supports by_duration, and the new regen commit fixes the Javadoc wrapping in the DSL factories. Good to merge once CI is green.
Claude Code on behalf of davsclaus
|
🧪 CI tested the following changed modules:
🔬 Scalpel shadow comparison — Scalpel: 9 of 697 tested, 24 compile-only — current: 9 all testedMaveniverse Scalpel detected 9 affected modules (current approach: 9). Skip-tests mode would test 9 modules (5 direct + 7 downstream), skip tests for 24 (generated code, meta-modules) Modules Scalpel would test (9)
Modules with tests skipped (24)
All tested modules (36 modules, 4m 6s total)Total reactor time: 4m 6s
Top 20 slowest modules:
|
6c54c95 to
b9cf758
Compare
gnodet-bot
left a comment
There was a problem hiding this comment.
Re-review after b9cf758 — squash/rebase with TUI test fix.
Previous findings status
All 9 mandatory findings from @davsclaus, @oscerd, and @Croway remain addressed — semantically identical to the previously approved 6c54c95.
Delta since 6c54c95
New file: PropertyCompletionProviderTest.java — the componentEnumOptionReturnsValues test was asserting autoOffsetReset enum values (latest, earliest), which no longer exist since autoOffsetReset changed from enum to string. Switched to compressionCodec (none, gzip) — still an enum, correct fix for the CI failure on the previous head.
Clean metadata-only PR. No functional change, no new code paths.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
b9cf758 to
8288623
Compare
gnodet-bot
left a comment
There was a problem hiding this comment.
Re-review after 8288623 — squash/rebase of previously approved content.
Previous findings status
All 9 mandatory findings from @davsclaus, @oscerd, and @Croway remain addressed — the diff is semantically identical to the previously approved b9cf758.
| # | Finding | Status |
|---|---|---|
| 1 | enums attribute removed (davsclaus) |
✅ |
| 2 | Generated files regenerated (davsclaus, oscerd) | ✅ |
| 3 | Stale description fixed (davsclaus) | ✅ |
| 4 | Test upgraded to catalog validation (davsclaus) | ✅ |
| 5 | JIRA issue created (davsclaus) | ✅ |
| 6 | DSL trailing spaces fixed (davsclaus) | ✅ |
| 7 | DefaultCamelContext closed via try-with-resources (davsclaus) |
✅ |
| 8 | topic added to validateProperties map (davsclaus) |
✅ |
| 9 | <ISO-8601> angle brackets replaced with PnDTnHnM (davsclaus) |
✅ |
TUI PropertyCompletionProviderTest fix (switched from autoOffsetReset to compressionCodec for enum test, added autoOffsetReset to string-returns-empty assertions) still present.
Clean metadata-only PR. No functional change, no new code paths.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
8288623 to
43c0770
Compare
gnodet-bot
left a comment
There was a problem hiding this comment.
Re-review after 43c0770 — squash/rebase of previously approved 8288623.
Previous findings status
All 9 mandatory findings from @davsclaus, @oscerd, and @Croway remain addressed — the diff is semantically identical to the previously approved 8288623.
| # | Finding | Status |
|---|---|---|
| 1 | enums attribute removed (davsclaus) |
✅ |
| 2 | Generated files regenerated (davsclaus, oscerd) | ✅ |
| 3 | Stale description fixed (davsclaus) | ✅ |
| 4 | Test upgraded to catalog validation (davsclaus) | ✅ |
| 5 | JIRA issue created (davsclaus) | ✅ |
| 6 | DSL trailing spaces fixed (davsclaus) | ✅ |
| 7 | DefaultCamelContext closed via try-with-resources (davsclaus) |
✅ |
| 8 | topic added to validateProperties map (davsclaus) |
✅ |
| 9 | <ISO-8601> angle brackets replaced with PnDTnHnM (davsclaus) |
✅ |
TUI PropertyCompletionProviderTest fix (switched from autoOffsetReset to compressionCodec for enum test, added autoOffsetReset to string-returns-empty assertions) still present.
Clean metadata-only PR. No functional change, no new code paths.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
davsclaus
left a comment
There was a problem hiding this comment.
The code is fine, but before merging we need documentation that lets both humans and AI tools understand what by_duration does and when it applies. Today the only documentation is the one-line option description, and that is not enough to use the option correctly.
Please add:
- A section in
components/camel-kafka/src/main/docs/kafka-component.adoc, e.g. "Where a new consumer group starts reading", that explains:autoOffsetResetis only used when the consumer group has no committed offset (or the committed offset is out of range). On a group that already has committed offsets it does nothing, soby_duration:PT5Mdoes not rewind an existing consumer by 5 minutes. This is the most common misunderstanding, and AI assistants get it wrong too.- What each value does:
earliest,latest(default),noneandby_duration:<ISO-8601 duration>, the last one requiring Kafka 4.0 or later. - How it differs from Camel's
seekTooption (BEGINNING/END), which applies on every start regardless of committed offsets.
- Examples in Java and YAML (canonical YAML DSL form), e.g. a new consumer group starting one hour back:
The doc examples are validated in the build and feed Camel's AI sample tooling, so they are what assistants will learn from.
from("kafka:orders?brokers=localhost:9092&groupId=reporting&autoOffsetReset=by_duration:PT1H")
- Wording in the option description (
KafkaConfiguration#setAutoOffsetReset, then regenerate):PnDTnHnMis not the full ISO-8601 form. Say "an ISO-8601 duration such asPT5MorP1D" instead, and add "only applies when the consumer group has no committed offset".
Remember to regenerate after changing the adoc and the description (catalog doc mirror, component/endpoint DSL). CI's generated-files check fails otherwise.
Claude Code on behalf of davsclaus
43c0770 to
63564be
Compare
gnodet-bot
left a comment
There was a problem hiding this comment.
Re-review after 63564be — squash/rebase with documentation added per @davsclaus's latest request.
Previous findings status
All mandatory findings from @davsclaus, @oscerd, and @Croway remain addressed.
| # | Finding | Status |
|---|---|---|
| 1 | enums attribute removed (davsclaus) |
✅ |
| 2 | Generated files regenerated (davsclaus, oscerd) | ✅ |
| 3 | Stale description fixed (davsclaus) | ✅ |
| 4 | Test upgraded to catalog validation (davsclaus) | ✅ |
| 5 | JIRA issue created (davsclaus) | ✅ |
| 6 | DSL trailing spaces fixed (davsclaus) | ✅ |
| 7 | DefaultCamelContext closed via try-with-resources (davsclaus) |
✅ |
| 8 | topic added to validateProperties map (davsclaus) |
✅ |
| 9 | <ISO-8601> angle brackets replaced with PnDTnHnM in description (davsclaus) |
✅ |
| 10 | TUI PropertyCompletionProviderTest switched to compressionCodec (CI fix) |
✅ |
New: documentation (davsclaus's latest CHANGES_REQUESTED)
| # | Request | Status |
|---|---|---|
| 1 | Section in kafka-component.adoc explaining autoOffsetReset semantics |
✅ "Where a new consumer group starts reading" section — clear explanation of when the option applies, correct emphasis that by_duration does not rewind existing groups |
| 2 | Java and YAML examples | ✅ Tabbed code blocks with by_duration:PT1H example |
| 3 | Wording in KafkaConfiguration#setAutoOffsetReset with "only applies when the consumer group has no committed offset" |
✅ Already present from previous iteration |
| 4 | seekTo distinction documented |
✅ NOTE at end of section clarifies seekTo vs autoOffsetReset |
The adoc section is well-written — the IMPORTANT callout about the most common misconception (thinking by_duration rewinds existing groups) is exactly the right thing to lead with. Both the component source adoc and catalog mirror are updated identically.
Clean metadata-only PR. No functional change, no new code paths.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
davsclaus
left a comment
There was a problem hiding this comment.
Thanks, the new "Where a new consumer group starts reading" section is exactly what was needed. The IMPORTANT box about existing groups, the value list, the Java/YAML tabs and the seekTo note are all good. A few things before this can go in:
- The YAML example is invalid (blocker).
steps:is placed next tofrom:underroute:, but the YAML DSL has nostepson a route; it belongs insidefrom:. People (and AI tools) copy these examples, so it has to be valid:- route: from: uri: kafka:orders parameters: brokers: "localhost:9092" groupId: reporting autoOffsetReset: "by_duration:PT1H" steps: - log: message: "Order received: ${body}"
- The escaped angle brackets are back in the option description:
by_duration:<ISO-8601 duration>. As noted earlier, the</>ends up verbatim in the catalog JSON (IDE tooltips,camel catalog) and double-escaped in the DSL javadoc. Please phrase it without brackets, e.g. "by_duration: followed by an ISO-8601 duration (e.g. by_duration:PT5M or by_duration:P1D; requires Kafka 4.0 or later)". - Minor: the description first says it applies when there is no initial offset or the stored offset is out of range, then "Only applies when the consumer group has no committed offset". Suggest: "Only applies when the consumer group has no committed offset, or the committed offset is out of range."
- Minor: in the adoc, "the offset whose timestamp is closest to
now - duration" would be more precise as "the first offset with a timestamp at or afternow - duration", which is how Kafka resolves it.
Please regenerate after the description change (catalog, component/endpoint DSL).
Claude Code on behalf of davsclaus
63564be to
61ead92
Compare
davsclaus
left a comment
There was a problem hiding this comment.
Thanks, the angle brackets, the "applies when" wording and the timestamp precision are all fixed. Two things left:
-
The YAML example is still invalid.
steps:is now indented underparameters:, so it becomes an endpoint parameter namedsteps, and the route has no steps. It must sit next toparameters:, insidefrom::- route: from: uri: kafka:orders parameters: brokers: "localhost:9092" groupId: reporting autoOffsetReset: "by_duration:PT1H" steps: - log: message: "Order received: ${body}"
Note that the PR doc validation job does not check the structure of YAML examples on component pages, so CI will not catch this. Please run the example (e.g. with
camel run) to be sure. -
The option description should start by saying what the option does. It now begins with "Only applies when…", and the first sentence is what tooltips and catalog listings often show on its own. Suggest:
Where a consumer group starts reading when it has no committed offset, or the committed offset is out of range. Valid values are: earliest (…), latest (…), none (…), by_duration: followed by an ISO-8601 duration (e.g. by_duration:PT5M or by_duration:P1D; requires Kafka 4.0 or later).
Then regenerate (catalog, component/endpoint DSL).
Claude Code on behalf of davsclaus
61ead92 to
4457e15
Compare
gnodet-bot
left a comment
There was a problem hiding this comment.
Re-review after 4457e15 — squash/rebase incorporating all documentation fixes requested by @davsclaus.
Previous findings status
All mandatory findings from @davsclaus, @oscerd, and @Croway remain addressed — the diff is semantically identical to the previously approved 63564be.
| # | Finding | Status |
|---|---|---|
| 1 | enums attribute removed (davsclaus) |
✅ |
| 2 | Generated files regenerated (davsclaus, oscerd) | ✅ |
| 3 | Stale description fixed (davsclaus) | ✅ |
| 4 | Test upgraded to catalog validation (davsclaus) | ✅ |
| 5 | JIRA issue created (davsclaus) | ✅ |
| 6 | DSL trailing spaces fixed (davsclaus) | ✅ |
| 7 | DefaultCamelContext closed via try-with-resources (davsclaus) |
✅ |
| 8 | topic added to validateProperties map (davsclaus) |
✅ |
| 9 | Angle brackets replaced with examples (davsclaus) | ✅ |
| 10 | TUI PropertyCompletionProviderTest switched to compressionCodec (CI fix) |
✅ |
| 11 | YAML example steps: under from: not parameters: (davsclaus) |
✅ Fixed — steps: now at same indent as parameters:, correctly nested under from: |
| 12 | Description starts with what the option does (davsclaus) | ✅ Fixed — "Where a consumer group starts reading when it has no committed offset..." |
| 13 | "applies when" wording includes out-of-range case (davsclaus) | ✅ |
| 14 | Timestamp precision "first offset with a timestamp at or after" (davsclaus) | ✅ |
Clean metadata-only PR. No functional change, no new code paths.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
gnodet-bot
left a comment
There was a problem hiding this comment.
Re-review after 56f795b — regen commit (Javadoc line-wrap adjustment in KafkaConfiguration.java).
Delta since 4457e15: 1 file, 2 lines — the word duration moved from line start to the end of the previous Javadoc line. This is the output of the project's regen script. Zero semantic change.
All previous findings from @davsclaus, @oscerd, and @Croway remain addressed.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
davsclaus
left a comment
There was a problem hiding this comment.
Thanks @smongiar. The only change since my approval at 4457e15 is the Regen commit, which re-wraps the setAutoOffsetReset Javadoc in KafkaConfiguration.java (formatter output, same text). The generated catalog JSON and DSL factories are unaffected, since the description is joined before wrapping.
I re-checked the PR head locally:
components/camel-kafkabuilds cleanly andKafkaConfigurationTestpasses (7/7). The working tree has no uncommitted generated changes afterwards.- The component and catalog copies of
kafka.jsonandkafka-component.adocare identical. - The Kafka client on
mainis 4.3.1. ItsAutoOffsetResetStrategy.fromStringrejects a bareby_durationand acceptsby_duration:followed by an ISO-8601 duration (Duration.parse, negative durations rejected). This matches the option description and the new doc section.
LGTM. The CI workflows for this head are waiting for maintainer approval (action_required), so they have not run yet. Please merge once they are green. The Regen commit message will go away with the squash merge.
This review does not replace specialized review tools or static analysis.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code on behalf of davsclaus
… value (Kafka 4.0+) Kafka 4.0 introduced the by_duration:<ISO-8601> auto offset reset strategy (KAFKA-18013), which seeks the consumer to the offset at (now - duration) when it starts. For example, by_duration:PT5M positions the consumer at the offset from 5 minutes before startup. Camel already passes autoOffsetReset through to ConsumerConfig without runtime validation, so by_duration:PT5M works on any build backed by kafka-clients 4.x (available since Camel 4.19 via CAMEL-23086). The only gap was documentation and tooling visibility. Changes: - Remove the enums attribute from @UriParam on autoOffsetReset so that by_duration:<ISO-8601> values pass catalog and camel validate checks (a bare enum entry "by_duration" would be invalid; the real form is parameterised and cannot be expressed as a fixed enum value) - Rewrite the autoOffsetReset description: drop the stale ZooKeeper reference, correct "fail" to "none", and add by_duration documentation - Regenerate the catalog mirror (camel-catalog/components/kafka.json) and update the endpoint-dsl and component-dsl factory javadocs - Add a catalog validation test using DefaultRuntimeCamelCatalog (already on classpath via camel-core, no new dependency needed) that by_duration:PT5M is accepted by validateProperties, verifying the end-to-end user-facing behaviour
56f795b to
b4b2dcf
Compare
gnodet-bot
left a comment
There was a problem hiding this comment.
Re-review after b4b2dcf — squash/rebase of previously approved 56f795b.
Previous findings status
All mandatory findings from @davsclaus, @oscerd, and @Croway remain addressed — the diff is semantically identical to the previously approved 56f795b.
| # | Finding | Status |
|---|---|---|
| 1 | enums attribute removed (davsclaus) |
✅ |
| 2 | Generated files regenerated (davsclaus, oscerd) | ✅ |
| 3 | Stale description fixed (davsclaus) | ✅ |
| 4 | Test upgraded to catalog validation (davsclaus) | ✅ |
| 5 | JIRA issue created (davsclaus) | ✅ |
| 6 | DSL trailing spaces fixed (davsclaus) | ✅ |
| 7 | DefaultCamelContext closed via try-with-resources (davsclaus) |
✅ |
| 8 | topic added to validateProperties map (davsclaus) |
✅ |
| 9 | Angle brackets replaced with examples (davsclaus) | ✅ |
| 10 | TUI PropertyCompletionProviderTest and YamlCompletionTest switched to compressionCodec (CI fix) |
✅ |
| 11 | YAML example steps: correctly nested under from: (davsclaus) |
✅ |
| 12 | Description starts with what the option does (davsclaus) | ✅ |
| 13 | "applies when" wording includes out-of-range case (davsclaus) | ✅ |
| 14 | Timestamp precision "first offset with a timestamp at or after" (davsclaus) | ✅ |
| 15 | Javadoc line-wrap in KafkaConfiguration.java (regen) | ✅ |
Clean metadata-only PR. No functional change, no new code paths.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
Summary
Kafka 4.0 introduced the
by_duration:<ISO-8601>auto offset reset strategy (KAFKA-18013), which seeks the consumer to the offset at(now - duration)when it starts. For example,by_duration:PT5Mpositions the consumer at the offset from 5 minutes before startup.Camel already passes
autoOffsetResetthrough toConsumerConfig.AUTO_OFFSET_RESET_CONFIGwithout runtime validation, soby_duration:PT5Mworks on any build backed bykafka-clients4.x (available since Camel 4.19 via CAMEL-23086). The only gap was documentation and tooling visibility.Changes
KafkaConfiguration.java: remove theenumsattribute from@UriParamonautoOffsetResetso thatby_duration:<ISO-8601>values pass catalog andcamel validatechecks — a bareby_durationenum entry would be invalid since the value is parameterised; rewrite the description to drop the stale ZooKeeper reference, correctfail→none, and document theby_durationsyntaxkafka.json(component + catalog mirror): update accordingly — type becomesstring(no fixed enum array), description updatedKafkaConfigurationTest: add unit test verifyingby_duration:PT5Mis passed through toConsumerConfig.AUTO_OFFSET_RESET_CONFIGas-isTesting
KafkaConfigurationTest#byDurationAutoOffsetResetIsPassedThroughToConsumerConfig— pure unit test, no container needed: setsautoOffsetReset=by_duration:PT5Mon aKafkaConfiguration, callscreateConsumerProperties(), and asserts the value reachesConsumerConfig.AUTO_OFFSET_RESET_CONFIGunchanged.