gnodet-bot commented on code in PR #27201:
URL: https://github.com/apache/camel/pull/27201#discussion_r4154951915
##########
components/camel-milo/src/main/java/org/apache/camel/component/milo/client/internal/SubscriptionManager.java:
##########
@@ -191,6 +197,13 @@ public void putSubscriptions(final Map<UInteger,
Subscription> subscriptions) th
} else {
final ReadValueId itemId = new ReadValueId(node,
AttributeId.Value.uid(), null, QualifiedName.NULL_VALUE);
final OpcUaMonitoredItem item = new
OpcUaMonitoredItem(itemId, MonitoringMode.Reporting);
+ if (null != s.getSamplingInterval()) {
Review Comment:
🔴 **`UInteger.valueOf()` on negative Integer — will throw unchecked
exception.** If a user configures `queueSize=-1` (e.g. by accident in a URI),
`UInteger.valueOf(-1)` throws `IllegalArgumentException` deep inside the
subscription setup. This will surface as an opaque connection failure, not a
clear configuration error.
Validate at configuration time or guard here:
```suggestion
if (null != s.getQueueSize()) {
if (s.getQueueSize() < 0) {
throw new IllegalArgumentException("queueSize
must be >= 0, got: " + s.getQueueSize());
}
item.setQueueSize(UInteger.valueOf(s.getQueueSize()));
}
```
##########
components/camel-milo/src/main/java/org/apache/camel/component/milo/client/MiloClientEndpoint.java:
##########
@@ -66,6 +66,12 @@ public class MiloClientEndpoint extends DefaultEndpoint {
@UriParam(defaultValue = "0.0")
private Double samplingInterval = 0.0;
+ /**
+ * The queue size used for subscriptions
+ */
+ @UriParam(description = "queue size for subscription")
+ private Integer queueSize;
Review Comment:
💡 **Missing default value and Javadoc quality:** The `@UriParam` has no
`defaultValue` and the description is lowercase/vague ("queue size for
subscription"). For consistency with the surrounding `samplingInterval` field
(which has `@UriParam(defaultValue = "0.0")`), set a sensible default and
capitalize the description.
Also, `queueSize` without a default means it's `null`, and the code guards
against it (`if (null != s.getQueueSize())`), so users who don't set it get the
milo library default. That's fine, but the Javadoc should say so explicitly.
```suggestion
/**
* The queue size used for OPC UA subscriptions. If not set, the OPC UA
server default is used.
*/
@UriParam(description = "Queue size for OPC UA subscriptions. If not
set, the server default is used.")
private Integer queueSize;
```
##########
components/camel-milo/src/test/java/org/apache/camel/component/milo/MonitorItemTest.java:
##########
@@ -45,6 +45,7 @@ public class MonitorItemTest extends AbstractMiloServerTest {
private static final String MILO_CLIENT_ITEM_C1_1 =
"milo-client:opc.tcp://foo:bar@localhost:@@port@@?node="
+
NodeIds.nodeValue(MiloServerComponent.DEFAULT_NAMESPACE_URI,
"myitem1")
+ +
"&requestedPublishingInterval=2000&samplingInterval=100&queueSize=10"
Review Comment:
⚠️ **Test doesn't actually verify parameter propagation.** The test URL now
includes `requestedPublishingInterval=2000&samplingInterval=100&queueSize=10`,
but the test body only asserts that 3 data values arrive — the same assertion
as before. This doesn't prove the parameters reached the OPC UA server. A
faster sampling interval (100ms) with a 200ms sleep (`2 * 100`) is actually
*more likely* to drop values than the old 1000ms/2000ms setup, because the
publishing interval is 2000ms — the server will batch 20 sampling intervals
into one publish cycle. The test passes by coincidence (3 values, each sent
200ms apart, all arrive within a single 2000ms publish cycle because
queueSize=10 holds them), not by design.
At minimum, add a comment explaining why this configuration proves the
parameters work, or better, assert something observable about the subscription
behavior (e.g. verify that with a very long publishing interval and a small
queue, values *are* dropped).
##########
components/camel-milo/src/main/java/org/apache/camel/component/milo/client/MonitorFilterConfiguration.java:
##########
@@ -32,7 +32,7 @@ public class MonitorFilterConfiguration implements Cloneable {
private MonitorFilterType monitorFilterType;
@UriParam(defaultValue = "0", description = "Deadband type for
MonitorFilterType DataChangeFilter.")
- private UInteger dataChangeFilterDeadbandType = UInteger.valueOf(0);
+ private Integer dataChangeFilterDeadbandType = Integer.valueOf(0);
Review Comment:
⚠️ **Null safety regression on `dataChangeFilterDeadbandType`.** The old
`UInteger` field was a value type initialized to `UInteger.valueOf(0)` — it
could never be `null`. The new `Integer` field is initialized to
`Integer.valueOf(0)`, but the setter accepts `null`. If someone calls
`setDataChangeFilterDeadbandType(null)` and then `createMonitoringFilter()`,
`UInteger.valueOf(null)` at line 84 will throw `NullPointerException`.
Either reject `null` in the setter or guard in `createMonitoringFilter()`.
##########
components/camel-milo/src/test/java/org/apache/camel/component/milo/MonitorItemTest.java:
##########
@@ -78,15 +79,16 @@ public void setup(TestInfo testInfo) {
}
/**
- * Monitor multiple events
+ * Monitor multiple events With explicit parameters for
requestedPublishingInterval, samplingInterval, and queueSize
*/
@Test
public void testMonitorItem1() throws Exception {
/*
- * we will wait 2 * 1_000 milliseconds between server updates since the
- * default server update rate is 1_000 milliseconds
+ * we will wait 2 * 100 milliseconds between server updates since the
Review Comment:
💡 **Typo:** "sexplicit" → "explicitly".
```suggestion
* we will wait 2 * 100 milliseconds between server updates since the
* explicitly set update rate is 100 milliseconds (samplingInterval).
* With longer requestedPublishingInterval and bigger queueSize we
should also get all updates.
```
##########
components/camel-milo/src/main/java/org/apache/camel/component/milo/client/internal/SubscriptionManager.java:
##########
@@ -191,6 +197,13 @@ public void putSubscriptions(final Map<UInteger,
Subscription> subscriptions) th
} else {
final ReadValueId itemId = new ReadValueId(node,
AttributeId.Value.uid(), null, QualifiedName.NULL_VALUE);
final OpcUaMonitoredItem item = new
OpcUaMonitoredItem(itemId, MonitoringMode.Reporting);
+ if (null != s.getSamplingInterval()) {
+ item.setSamplingInterval(s.getSamplingInterval());
+ }
+ if (null != s.getQueueSize()) {
Review Comment:
💡 **`setFilter(null)` called unconditionally.** When no
`monitorFilterConfiguration` is set (the common case — most users don't
configure data change filters), `createMonitoringFilter()` returns `null`, so
this calls `item.setFilter(null)`. On the current milo SDK this is likely a
no-op, but it's unnecessary and fragile if the SDK changes its null-handling.
Guard it:
```suggestion
MonitoringFilter filter = s.createMonitoringFilter();
if (filter != null) {
item.setFilter(filter);
}
```
--
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]
