vineetsaurabh commented on PR #29489: URL: https://github.com/apache/camel/pull/29489#issuecomment-6766480753
> ### Critical > **F1 — Wrong `firstVersion`:** The `@UriEndpoint` annotation sets `firstVersion = "4.16.0"`, but this component is being introduced in `4.24.0-SNAPSHOT`. The `firstVersion` must reflect the release in which the component is first shipped. Update to match the actual target release. Already covered in [davsclaus](https://github.com/davsclaus) suggestions. > **F2 — `subscribeToDesiredProperties` called on every message:** `TwinDeliveryStrategy.execute()` is invoked for every message exchange; it calls `client.subscribeToDesiredProperties(...)` with a no-op callback before fetching the twin. This is a SDK subscription call, not a getter — it should be called once during client initialization (e.g. in `DeviceClientPool.createClient`), not on every send. As written, every twin update re-registers a callback and adds overhead. > Already covered in [davsclaus](https://github.com/davsclaus) suggestions. > **F3 — Blocking I/O inside `ConcurrentHashMap.compute`:** `pool.compute(deviceId, ...)` calls `createClient(...)` which calls `client.open(true)` — a blocking network I/O operation — inside the lambda. `ConcurrentHashMap.compute` holds the bin lock for the hash bucket for the entire lambda duration. Any thread accessing a key in the same bucket will block until the connection is established. This can cause cascading delays and potential deadlocks under load. Use a two-phase approach: check/create outside `compute`, or use `computeIfAbsent` with a lightweight wrapper that defers the actual `open()` call. > Already covered in [davsclaus](https://github.com/davsclaus) suggestions. > ### Important > **F4 — Use-after-eviction race:** `getOrCreate` returns a `DeviceClient` reference, then the caller proceeds to use it. Meanwhile, the eviction thread may call `evictIdleClients()`, acquire the entry reference, and call `close()` on the same client — leaving the caller with a closed connection. `touch()` after `compute()` (line 1277) does not prevent this: the evictor may have already captured the entry before the touch. Consider holding a "lease" on the client during use (e.g. reference counting, or returning the `PooledClient` and calling `touch` per-use with a guard in the evictor), or at minimum document the race as a known limitation. > I addressed it by tracking a per-client in-use lease while the client is being used, and the eviction thread now skips clients that are currently leased. That prevents the eviction path from closing a client that is still in active use without changing the existing pooling behavior. > **F5 — `CONNECTION_STRING` header missing `secret = true`:** The `@Metadata` annotation for `CONNECTION_STRING` lacks `secret = true`. The generated JSON confirms `"secret": false` for this header. The corresponding `@UriParam` on the configuration field correctly has `secret = true`. Since the header carries the same sensitive credential, the annotation should be consistent. > I aligned the header metadata with the endpoint configuration by marking CamelAzureIoTHubConnectionString as secret = true; this does not change runtime behavior but keeps the generated metadata consistent with the sensitive connection-string handling. > ### Minor > **F6 — Duplicate headers include in docs:** `component-endpoint-headers.adoc` is included twice in `azure-iothub-component.adoc`. Once inside the `// component options: START/END` block (line 55) — which is incorrect placement — and once standalone after `// endpoint options: END` (line 62). Remove the first occurrence. > I removed the duplicate component-endpoint-headers.adoc include from the // component options: START/END block and kept the single standalone include in the correct place. > **F7 — Missing `MojoHelper` registration:** For multi-module Azure components, new sub-components typically need to be registered in `MojoHelper.java` so the catalog and docs generation tooling picks them up. This file is not modified in the PR — please verify whether registration is required, or explain why it is not needed here. > Already covered in [davsclaus](https://github.com/davsclaus) suggestions. > ### Low > **F9 — `Thread.sleep` in test:** `DeviceClientPoolTest` uses `Thread.sleep(10)` in a polling loop with a bounded timeout. This is a timing-dependent test that may be flaky under slow CI. Consider using `Awaitility` (already used in other Camel tests) for the assertion. > Already covered in [davsclaus](https://github.com/davsclaus) suggestions. -- 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]
