ramu11 commented on PR #26006:
URL: https://github.com/apache/camel/pull/26006#issuecomment-5503439843
> A well-structured new MQTT component following standard Camel patterns
with good test coverage and documentation. Nice work on the `SendDynamicAware`
implementation and the consumer exchange release guard.
>
> **Highlights:**
>
> * Component follows the standard Component/Endpoint/Producer/Consumer
class hierarchy correctly
> * Security annotations are correct: `password` is marked `secret = true`
> * Test infra uses `mirror.gcr.io/hivemq/hivemq-ce:2025.5` (follows project
guidelines)
> * No MojoHelper registration needed (direct child of `components/`)
> * No header injection risk (consumer only sets headers from structured
MQTT protocol fields)
> * `supportLevel: Preview` is appropriate for a new component
>
> ### Verified Findings
> **1. Subscribe call missing timeout** (medium)
>
> In `HiveMQConsumer.doStart()`, `client.subscribeWith()...send().join()`
has no `.orTimeout()` call. In contrast, `HiveMQEndpoint.connect()` properly
uses `.orTimeout(HiveMQConstants.DEFAULT_CONNECT_TIMEOUT_SECONDS,
TimeUnit.SECONDS)` on the connect future. If the broker accepts the TCP
connection but delays or drops the SUBACK, route startup will block
indefinitely on `.join()`. Consider adding `.orTimeout()` to the subscribe
chain for consistency.
>
> **2. Bare `RuntimeException` in `copy()`** (low)
>
> `HiveMQConfiguration.copy()` (line 159) wraps `CloneNotSupportedException`
in `new RuntimeException(e)`. Other Camel components with the same clone
pattern (Minio, Caffeine, Webhook, SQS2, DDB2, etc.) all use
`RuntimeCamelException`. This component's own
`HiveMQEndpoint.unwrapConnectFailure()` already uses `RuntimeCamelException`,
so this is inconsistent within the component itself.
>
> **3. Inconsistent `@SuppressWarnings("deprecation")`** (low)
>
> `HiveMQQosAndRetainIT` applies `@SuppressWarnings("deprecation")` on calls
to deprecated `getMqttHost()`/`getMqttPort()`, but other tests
(`HiveMQComponentPubSubIT`, `HiveMQEmptyPayloadIT`, `HiveMQOverrideTopicIT`,
`HiveMQSendDynamicIT`) call the same deprecated methods without suppression.
Consider migrating all tests to the non-deprecated `brokerUrl()` API.
>
> ### 📋 PR Metadata
> Aspect Current Suggested
> Labels `components`, `core`, `catalog`, `docs`, `dsl`,
`core-build-and-dependencies` Remove `core`, `core-build-and-dependencies`
(only auto-generated files touch core)
> Milestone _(none)_ `4.23.0`
> Category — `feature`
> _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_
Thanks for the review. All three comments have been addressed:
1. Added the subscribe timeout using `.orTimeout(...)`.
2. Replaced `RuntimeException` with `RuntimeCamelException` in `copy()`.
3. Added the required `@SuppressWarnings("deprecation")` annotations to the
affected tests.
The changes have been committed and pushed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
