gnodet-bot commented on code in PR #27173:
URL: https://github.com/apache/camel/pull/27173#discussion_r4171984399
##########
components/camel-sjms/src/main/java/org/apache/camel/component/sjms/SjmsEndpoint.java:
##########
@@ -449,6 +488,35 @@ public MessageListenerContainer
createMessageListenerContainer(SjmsEndpoint endp
return answer;
}
+ public BatchMessageListenerContainer createBatchMessageListenerContainer(
+ SjmsEndpoint endpoint) {
+ BatchMessageListenerContainer answer = new
BatchMessageListenerContainer(endpoint);
+ answer.setConcurrentConsumers(concurrentConsumers);
+ return answer;
+ }
+
+ private void validateBatchingOptions() {
+ if (getExchangePattern().isOutCapable()) {
+ throw new IllegalArgumentException("SjmsConsumer does not support
exchangePattern=InOut in batching mode");
+ }
+
+ if (getBatchInterval() < 0) {
+ if (getExchangePattern().isOutCapable()) {
Review Comment:
🔴 **Still present: negative `batchInterval` validation is dead code.** The
`isOutCapable()` check at line 497 already threw before reaching this block —
so the inner `isOutCapable()` guard at line 500 can never be true. Also,
`batchSize <= 0` is still not rejected.
This was flagged in the previous review by both gnodet-bot and @davsclaus.
The correct fix:
```suggestion
if (getBatchInterval() < 0) {
throw new IllegalArgumentException("batchInterval must be 0 or
greater.");
}
if (getBatchSize() <= 0) {
throw new IllegalArgumentException("batchSize must be greater
than 0.");
}
```
##########
components/camel-sjms/src/test/java/org/apache/camel/component/sjms/batch/BatchTestSupport.java:
##########
@@ -0,0 +1,74 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.camel.component.sjms.batch;
+
+import java.util.List;
+
+import org.apache.camel.Exchange;
+import org.apache.camel.ProducerTemplate;
+import org.apache.camel.component.mock.MockEndpoint;
+import org.apache.camel.component.sjms.*;
Review Comment:
⚠️ **Still present: star import.** `org.apache.camel.component.sjms.*` — the
project uses explicit imports. The Regen commit removed the unused
`BatchEndpointMessageListener` import but left this star import. This was
already flagged by @davsclaus.
```suggestion
import org.apache.camel.component.sjms.SjmsConstants;
```
##########
components/camel-sjms/src/test/java/org/apache/camel/component/sjms/batch/BatchTestSupport.java:
##########
@@ -0,0 +1,74 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.camel.component.sjms.batch;
+
+import java.util.List;
+
+import org.apache.camel.Exchange;
+import org.apache.camel.ProducerTemplate;
+import org.apache.camel.component.mock.MockEndpoint;
+import org.apache.camel.component.sjms.*;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertInstanceOf;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+
+@SuppressWarnings("unchecked")
+public final class BatchTestSupport {
+
+ private BatchTestSupport() {
+ }
+
+ public static void sendMessages(ProducerTemplate template, String
endpoint, int count) {
+ for (int i = 0; i < count; i++) {
+ String message = "Hello World " + i;
+ template.sendBody(endpoint, message);
+ }
+ }
+
+ /** Extracts the batch body as a List<Exchange> from a batch Exchange,
asserting the type. */
+ public static List<Exchange> getBatch(Exchange batchExchange) {
+ List<Exchange> body = batchExchange.getIn().getBody(List.class);
+ assertNotNull(body, "batch exchange body was null");
+ for (Object o : body) {
+ assertInstanceOf(Exchange.class, o, "batch element was not an
Exchange: " + o);
+ }
+ return body;
+ }
+
+ /** Asserts a single batch exchange has exactly the given number of
messages. */
+ public static void assertBatchSize(Exchange batchExchange, int
expectedSize) {
+ List<Exchange> batch = getBatch(batchExchange);
+ assertEquals(expectedSize, batch.size(), "unexpected batch size");
+ assertEquals(expectedSize,
+
batchExchange.getIn().getHeader(SjmsConstants.SJMS_BATCH_SIZE_HEADER,
Integer.class),
+ "CamelSjmsBatchSize header did not match actual batch size");
+ }
+
+ /**
+ * Asserts the mock received exactly expectedSizes.length batch exchanges,
in order, with each batch's size matching
+ * the corresponding element. Requires concurrentConsumers=1 (or an
otherwise deterministic single-worker setup) so
+ * that arrival order is meaningful.
+ */
+ public static void assertBatchSizesInOrder(MockEndpoint mock, int...
expectedSizes) {
+ assertEquals(mock.getExchanges().size(), expectedSizes.length, "More
mock exchanges than provided batch sizes.");
Review Comment:
⚠️ **Still present: `assertEquals` arguments swapped.** JUnit 5's signature
is `assertEquals(expected, actual, message)`. Here `mock.getExchanges().size()`
(the actual value) is in the expected position. This was already flagged by
@davsclaus.
```suggestion
assertEquals(expectedSizes.length, mock.getExchanges().size(), "More
mock exchanges than provided batch sizes.");
```
##########
components/camel-sjms/src/main/java/org/apache/camel/component/sjms/SjmsConstants.java:
##########
@@ -38,5 +38,8 @@ public interface SjmsConstants {
description = "Provides an explicit ReplyTo destination
(overrides any incoming value of Message.getJMSReplyTo() in consumer)",
javaType = "String")
String JMS_REPLY_TO = JmsConstants.JMS_REPLY_TO;
-
+ @Metadata(label = "consumer, batch",
+ description = "The size of the batch when using the batching
consumer option.",
+ javaType = "String")
Review Comment:
⚠️ **Still present: header metadata issues.** The `javaType` should be
`"Integer"` (the header carries a batch size count, not a String), and the
label has a leading space (`"consumer, batch"`) which causes the generated JSON
to produce `"group": " batch"` with a space. This was flagged by @davsclaus.
```suggestion
@Metadata(label = "consumer,batch",
description = "The size of the batch when using the batching
consumer option.",
javaType = "Integer")
```
--
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]
