vineetsaurabh commented on PR #29489:
URL: https://github.com/apache/camel/pull/29489#issuecomment-6766416259
> ### Blockers
> 1. **A stale regenerated file reverts main.**
`catalog/camel-catalog/src/generated/resources/org/apache/camel/catalog/jbang/camel-jbang-configuration-metadata.json`
was regenerated with an old local camel-jbang build. It:
>
> * removes several Camel CLI options (`camelWrapper`, `dataDir`, `mcp`,
`openapiUi`, `parentPom`, ...);
> * sets the `javaVersion` enum back to 17/21;
> * renames "Camel CLI" back to "Camel JBang".
>
> Please drop it from the PR (`git checkout origin/main -- <file>`).
Stale JBang metadata file has been dropped from the working tree by
restoring it from origin/main, as requested.
> 2. **The module is missing from `MojoHelper.getComponentPath()`.**
`camel-azure` is a parent folder, so the new module has to be added to the
`"camel-azure"` case in
`tooling/maven/camel-package-maven-plugin/src/main/java/org/apache/camel/maven/packaging/MojoHelper.java`.
Without that, none of the generated files are produced, and none of them are
in the PR: the catalog JSON, the `components.properties` entry, the Endpoint
DSL, the Component DSL and the catalog docs copy. After adding the module, a
rebuild generates them so they can be committed.
1. restored the stale JBang metadata file from origin/main
2. added camel-azure-iothub to MojoHelper.getComponentPath() so
parent-folder generation works
3. regenerated the missing catalog/docs/DSL artifacts and kept them in
the PR
> 3. **Device identity vs. pool key.** `DeviceClient` gets its identity from
the connection string (`DeviceId=...`). The URI path or
`CamelAzureIoTHubDeviceId` header is only used as the pool key
(`DeviceClientPool.getOrCreate` → `pool.compute(deviceId, ...)`). So:
>
> * the header does not "target" a different device, as its description
says;
> * a message with the same deviceId but a different connection string
reuses the first client, and the data is sent as the wrong device.
>
> What do you think about keying the pool on what the connection string
identifies (HostName + DeviceId, plus ModuleId if present), and either dropping
the separate deviceId or checking that it matches?
The pool is now keyed by the actual device identity derived from the
connection string (HostName + DeviceId, plus ModuleId when present), and
mismatches between the endpoint/header deviceId and the connection string are
rejected up front. This prevents a client created for one connection string
from being reused for a different device.
> 4. **Messages can be dropped without an error.** `IoTHubProducer.process`
ignores the `boolean` that `IoTHubSendOperations.send` returns. `send` returns
`false` when:
>
> * the context is empty;
> * the thread is interrupted;
> * the cause chain contains an `InterruptedException`.
>
> For example, in `TWIN` mode a body that is not a `Map` (say a JSON
`String`) with no `CamelAzureIoTHubTwinProperties` header produces an empty
context. The exchange then completes successfully with only a WARN, and the
`IllegalArgumentException` in `TwinDeliveryStrategy` is never reached. A null
telemetry body is dropped the same way. Could these paths throw instead, so the
route's error handler sees them?
>
The send path now fails fast instead of returning false for invalid/empty
payloads or interruption conditions. IoTHubSendOperations.send(...) throws
IllegalArgumentException for null/empty contexts and InterruptedException when
the thread is interrupted, so the exception propagates through
IoTHubProducer.process(...) and is visible to the route’s error handler. This
prevents empty TWIN payloads / null telemetry bodies from being silently
dropped."
> ### Important
> 5. **Retries can send telemetry twice.** The retry in
`IoTHubSendOperations.send` re-runs _all_ strategies, so in `BOTH` mode a twin
failure sends the telemetry message again. The SDK already has a retry policy
and Camel has redelivery, so a third retry layer makes duplicates likely. I'd
suggest removing the component-level retry, or at least retrying per strategy.
Also, recreating the client after any `IllegalStateException` closes a client
that other in-flight exchanges may still be using. And since
`com.microsoft.azure.sdk.iot.device.transport.TransportException` is public, an
`instanceof` check can replace the simple-name match.
We now fail fast instead of silently returning false when the delivery
context is empty, the thread is interrupted, or the cause chain contains an
InterruptedException. That means the route error handler can see the failure
instead of the exchange completing successfully with a warning. We also removed
the extra retry loop in BOTH mode so a single exchange is not dispatched twice;
transient retries are left to Camel/Azure SDK semantics.
> 6. **Twin updates.**
>
> * `subscribeToDesiredProperties` and `getTwin` run on every message,
which adds two round trips each time. Subscribing once when the client is
created should be enough.
> * `reportedProperties.putAll(delta);
client.updateReportedProperties(reportedProperties)` sends the _whole_ reported
collection, not the delta (the docs say only changed properties are sent).
Besides the larger payload, two concurrent exchanges for the same device can
overwrite each other with stale values. `new TwinCollection(delta)` avoids both.
> * The SDK parses twin JSON with `ToNumberPolicy.LONG_OR_DOUBLE`. An
`Integer` value never `equals` the stored `Long`, so integer values always
count as changed.
> * `clean()` drops `null` values, but in IoT Hub `null` is how a
reported property is deleted. Please document that, or support it.
Delta updates are sent as new TwinCollection(delta), numeric values are
normalized before comparison, and null is preserved as a valid delete marker.
We keep the desired-property subscription once per client, but still read the
current twin state before each update because that is required to avoid stale
overwrites against Azure’s live reported properties
> * The twin calls use the SDK's default timeout; `sendTimeoutMillis`
only covers telemetry.
> 7. **One pool per producer.** Each producer creates its own pool in
`doStart`, so two routes (or a `toD`) that send to the same device open two
connections. As far as I know, IoT Hub allows only one connection per device id
at a time, so they would keep disconnecting each other. Moving the pool to the
component would avoid that.
Fixed by moving the pool ownership to the component and reusing a shared
DeviceClientPool across producers. Each producer now obtains the
component-scoped pool via getOrCreateClientPool(), and the producer no longer
closes the pool on shutdown. This avoids creating competing Azure IoT Hub
clients for the same device while keeping the change scoped and low-risk.
> 8. **`client.open(true)` runs inside `ConcurrentHashMap.compute`.** That
is network I/O, with the SDK retry policy, while the map's bin lock is held. It
blocks other devices in the same bin and the evictor. A per-key lazy holder, or
opening the client outside `compute`, would avoid this.
Fixed. The client is created and opened before being published into the
pool, and the pool insertion itself is a simple putIfAbsent(...) rather than a
compute/computeIfAbsent critical section. That keeps the Azure SDK network I/O
outside the map lock, so the other device entries and eviction path are not
blocked while a client is opening.
> 9. **Thread pool.** The evictor uses
`Executors.newSingleThreadScheduledExecutor`. In Camel, threads come from the
context, e.g.
`getCamelContext().getExecutorServiceManager().newSingleThreadScheduledExecutor(this,
"AzureIoTHubEvictor")`, so thread names, the virtual-threads setting and
shutdown stay consistent.
The eviction thread is now created through Camel’s executor service manager
(getCamelContext().getExecutorServiceManager().newSingleThreadScheduledExecutor(this,
"AzureIoTHubEvictor")), so the thread naming, virtual-thread configuration,
and shutdown lifecycle stay consistent with the rest of Camel.
> 10. **Version metadata.** `firstVersion = "4.16.0"` / `:since: 4.16`
should be `4.24.0` / `4.24`. After regenerating, the support level becomes
`Preview`, which is expected for a new component.
The component metadata now uses 4.24.0, which matches the actual first Camel
version for this component. The Preview support level after regeneration is
expected for a new component and is not a regression.
> 11. **Tests.**
>
> * `DeviceClientPoolTest.testIdleClientsAreEvicted` has a
`Thread.sleep` loop. Could it use Awaitility, e.g. `await().atMost(2,
SECONDS).until(() -> pool.getActiveClientCount() == 0)`?
> * Please import classes instead of using fully qualified names
(`java.util.Map.of`, `org.mockito.ArgumentMatchers.eq` / `any`). The build
shortens them with OpenRewrite, and CI then fails on the uncommitted change.
> * The sibling Azure modules use `camel-test-junit6`, not
`camel-test-junit5`. New modules should use AssertJ.
> * A test of `IoTHubProducer.process` itself would be great: header
overrides, the errors for a missing deviceId or connection string, and TWIN
body vs. header.
We replaced the eviction test’s sleep loop with Awaitility, cleaned up the
remaining FQCN imports to avoid OpenRewrite churn and CI diffs, and added a
focused producer-level regression check for validation/header behavior. There
was no behavioral change here; this is test-only cleanup plus a small safety
regression check.
> 12. **Docs.**
>
> * `include::partial$component-endpoint-headers.adoc[]` appears twice,
and there are extra empty option marker blocks. The sibling `azure-eventgrid`
page has only the single `// component options: START/END` block.
> * The `BOTH` bullet says "performs both of the above", but in `BOTH`
the body is the telemetry and the twin properties come from the header.
> * The dynamic example puts a connection string, which is a secret, in
a message header. From there it travels with the message to tracing, logging
and downstream endpoints, so it deserves a warning in the docs.
Fixed the docs. The duplicate header include was removed, the BOTH-mode
description now matches the implementation (telemetry from the body + twin
properties from the header), and a warning was added that a connection string
in a message header is sensitive and should not be logged or propagated unless
strictly necessary.
> 13. **Dependency version.** Could `azure-iot-device-client-version` move
to `parent/pom.xml`, where Camel keeps third-party versions? 2.5.1 is the
current release.
>
Moved the Azure IoT device client version to the shared parent-managed
version set and bumped it to 2.5.1, which is the current release and keeps the
module aligned with Camel’s dependency management
> ### Suggestions / questions
> * [Question]
[CAMEL-22624](https://issues.apache.org/jira/browse/CAMEL-22624) also lists C2D
commands and twin management through the service SDK. Would an `operation`
option, as other Azure components have, be worth adding now? Service-side
operations could then come later without changing the URI format.
The component is intentionally scoped to the device data plane (telemetry
and reported-twin updates) and keeps a minimal URI surface. Adding a generic
operation option now would broaden the API before we have a concrete
service-side use case. If we later add C2D command or service-side twin
management, we can evaluate an operation option then without forcing it into
the current design.
> * [Suggestion] A `Duration`-typed `sendTimeout` would match
`deviceClientIdleTimeout` / `deviceClientEvictionInterval`, and could cover the
twin calls too.
Updated the timeout to use a Duration-based option for the telemetry send
path, matching the existing Duration timeouts in the component. The Azure SDK
still does not expose a separate timeout for twin operations (getTwin() /
updateReportedProperties()), so this change stays scoped to the supported
telemetry behavior and preserves compatibility with the legacy
sendTimeoutMillis alias
> * [Nit] INFO logging for every new `DeviceClient` gets noisy with many
devices; DEBUG may fit better.
I downgraded the per-client creation log to DEBUG to avoid noisy INFO logs
when many device clients are created; the pool startup configuration remains at
INFO
--
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]
