davsclaus commented on code in PR #27201:
URL: https://github.com/apache/camel/pull/27201#discussion_r4163318623
##########
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 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.")
Review Comment:
Nit (optional, non-blocking): the `queueSize` description still says "If not
set, the server default is used". That isn't quite what happens: milo's
`OpcUaMonitoredItem` defaults `queueSize` to `uint(1)`, so the client asks for
a queue size of 1. The `description` attribute is also redundant, because the
tooling takes it from the Javadoc like the other fields here. Could you change
it to:
```suggestion
* The queue size used for OPC UA subscriptions. If not set, a queue
size of 1 is requested.
*/
@UriParam
```
(and then regenerate milo, catalog and endpoint-dsl again)
##########
components/camel-milo/src/main/java/org/apache/camel/component/milo/client/internal/SubscriptionManager.java:
##########
@@ -191,6 +197,19 @@ 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()) {
+ if (s.getQueueSize() < 0) {
+ throw new IllegalArgumentException("queueSize must
be >= 0, got: " + s.getQueueSize());
+ }
Review Comment:
Nit (optional, non-blocking): `MiloClientEndpoint.setQueueSize` already
rejects negative values, so this second check can't trigger. It could be
removed to keep the subscription code lean.
##########
docs/user-manual/modules/ROOT/pages/camel-4x-upgrade-guide-4_23.adoc:
##########
@@ -4173,6 +4173,18 @@ scan for nothing. A route file inside a dot directory is
therefore no longer wat
within two seconds rather than up to ten. A new `setStableTimeout` (default
200 milliseconds) leaves a file that was
only just modified for the next scan, so a save still being written is not
reloaded half-finished.
+=== camel-milo - potential breaking change
Review Comment:
Nit (optional, non-blocking): because the section was appended at the end of
the file, it now sits under the `== Route reload` heading. Could you move it up
next to the other component entries (for example after `=== camel-jms -
request/reply ...`, around L4139)? Please also put `samplingInterval`,
`requestedPublishingInterval`, `dataChangeFilterDeadbandType`,
`UInteger`/`Integer`, `MonitorFilterConfiguration` and `OpcUaMonitoredItem` in
backticks, like the rest of the guide.
##########
components/camel-milo/src/test/java/org/apache/camel/component/milo/MonitorItemTest.java:
##########
@@ -78,20 +83,22 @@ 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
+ * explicitly set update rate is 100 milliseconds (samplingInterval)
+ * With 2000ms requestedPublishingInterval and bigger queueSize of 4
we should get all updates,
*/
- final var time = 2 * 1_000;
+ final var time = 2 * 100;
final var timeout = 10 * 1_000; // 10 seconds timeout for assertions
// item 1 ... only this one receives
test1Endpoint.reset();
- test1Endpoint.setExpectedCount(3);
+ test1Endpoint.setMinimumExpectedMessageCount(5); // the first 3,
plus at least 4 more from rest (if they fall to 1 period)
+ test1Endpoint.setAssertPeriod(timeout);
Review Comment:
Nit (optional, non-blocking): two comment fixes here: "bigger queueSize of 4
we should get all updates," (L93) has a trailing comma, and "the first 3, plus
at least 4 more" doesn't match `setMinimumExpectedMessageCount(5)`. Also,
`setAssertPeriod(timeout)` adds a fixed 10 s to the test; a shorter period (for
example 3 s, longer than one publishing interval) would be enough.
--
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]
