vineetsaurabh commented on PR #29489:
URL: https://github.com/apache/camel/pull/29489#issuecomment-6773959747
> 1. **A null client can get stuck in the pool.** `createClient` returns
`null` on interruption (`DeviceClientPool.java:305-309`), and `acquire` caches
it (`:186-187`). Every later send for that device then fails with an NPE. Each
failed send also touches the entry, so the idle evictor never removes it. Could
`createClient` throw instead (re-interrupt the thread first)? Also, if
`subscribeToDesiredProperties` (`:300`) fails after `open(true)`, the opened
client is never closed.
Fixed: client creation now fails fast on interruption instead of caching a
null client, and any client opened before a failed
subscribeToDesiredProperties() setup is closed immediately to avoid leaking
half-open connections.
> 2. **Two connections for one device** (gnodet-bot's thread at `:187`). Two
first sends for the same device both open a client before `putIfAbsent`, and
IoT Hub drops one of the two connections. A per-key holder avoids this, e.g.
`ConcurrentHashMap<String, CompletableFuture<PooledClient>>` with
`computeIfAbsent`, opening the client outside the map.
Fixed: client creation is now serialized per device key before
createClient() is invoked, preventing two threads from concurrently opening two
Azure IoT Hub connections for the same device and eliminating the
duplicate-connect race
> 3. **Telemetry body conversion** (`TelemetryDeliveryStrategy.java:60-70`).
Only `byte[]`/`String` are accepted. Stream caching is on by default, so an
HTTP or file body arrives as `InputStreamCache` and fails.
`exchange.getIn().getMandatoryBody(byte[].class)` in the producer would cover
all the type converters.
>
Fixed: telemetry now resolves through
exchange.getIn().getMandatoryBody(byte[].class) before dispatch, so Camel’s
converter chain handles InputStream/InputStreamCache payloads as expected. The
delivery strategy remains strict and only accepts the resolved byte[] payload.
> **Important**
> 4. **Eviction vs. acquire.** Between `pool.get` (`:178`) and
`existing.acquire()` (`:181`), the evictor can still see `inUse == 0` with an
old `lastUsed`, then remove and close the client (`:333-335`). Doing the
acquire inside `pool.computeIfPresent(...)`, and the eviction as
`computeIfPresent(key, (k, v) -> v.inUse.get() == 0 && idle ? null : v)` with
the close afterwards, makes both atomic. Neither lambda does any I/O.
Fixed: the idle-eviction check and the in-use transition are now performed
atomically per device key using computeIfPresent, preventing an evictor from
closing a client while another thread is acquiring it. The actual close still
happens after the map update, outside the lambda.
> 5. **Broken clients are never replaced.** The Javadoc (`:40-41`) says send
operations remove broken clients, but they no longer do. The SDK does expose
connection state: `setConnectionStatusChangeCallback`. Once the SDK stops
reconnecting, every send fails until the client has been idle for 5 minutes,
which never happens under steady traffic. What do you think about evicting on a
terminal `DISCONNECTED` status?
I fixed this by evicting the client on the SDK’s terminal DISCONNECTED
callback, not only on idle timeout. This prevents stale clients from being
reused indefinitely under steady traffic. Added a regression test for the
callback path
> 6. **Ignored endpoint options.** The pool is built from the component
configuration only. So `deviceClientIdleTimeout` and
`deviceClientEvictionInterval` on an endpoint URI
(`IoTHubConfiguration.java:60-66`) have no effect. Could they become component
options?
These are pool-level settings rather than endpoint-level settings. The
component builds one shared DeviceClientPool from the component configuration,
so URI values for deviceClientIdleTimeout and deviceClientEvictionInterval are
ignored. Keeping them as component options matches the actual behavior.
>7. **Tests.**
> * The module still uses `camel-test-junit5` (`pom.xml:51`,
`IoTHubComponentTest`); all sibling Azure modules use `camel-test-junit6`.
> * `DeviceClientPoolTest` only covers `getOrCreate`, which production does
not call. A test of the `Lease` path would help: in-use skip, release, and a
concurrent first acquire.
> * A test of `IoTHubProducer.process` itself is still missing.
>
I fixed the test gaps by:
* switching the module from camel-test-junit5 to camel-test-junit6
* adding lease-path coverage for:
* in-use reuse / release behavior
* concurrent first acquire creating a single client
* adding a producer-level regression for IoTHubProducer.process(...) with a
stream-backed telemetry body to verify conversion and send semantics
> **Minor**
>
> * `touchAndMaybeEvict` (`:314-319`) touches the client and then compares
`now - now`, so the eviction branch never runs. Only touching on release is
enough, because the evictor already handles idle clients.
Already covered
> * In `BOTH` mode without the twin header, telemetry is sent first and then
`TwinDeliveryStrategy` throws, so a redelivery sends the telemetry twice. Could
the producer validate this before sending?
I fixed it by validating the required twin header before dispatching
telemetry, so invalid BOTH requests fail fast and do not send telemetry before
the twin validation error.
> * In `clean()`, an empty nested map becomes `null`
(`TwinDeliveryStrategy.java:102-104`, `:112-113`), which deletes the whole
nested property in IoT Hub.
An empty nested map was being converted to null, and IoT Hub interprets that
as deleting the whole nested property. I fixed it so empty nested maps are
preserved as empty objects rather than being turned into null, and added a
regression test covering that case.”
> * The SDK has timeout overloads for twin calls: `getTwin(int)`,
`updateReportedProperties(TwinCollection, int)`. So `sendTimeout` could cover
twin updates too, and its description (`IoTHubConfiguration.java:52`) is not
accurate.
I updated the implementation to use sendTimeout for both telemetry sends and
twin reported-property updates, and corrected the configuration wording
accordingly
> * A new component does not need the deprecated `sendTimeoutMillis` alias
(`IoTHubConfiguration.java:55-58`).
I removed it and kept only the sendTimeout option.
> * Unused public API could go: `getOrCreate`, `lease`/`acquire`,
`getClient`/`client`, `remove(...)`. The no-op `IoTHubProducer.doStop` can go
too.
Trimmed the API back to the minimal required surface so the component
exposes only the behavior it actually uses.
--
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]
