davsclaus commented on code in PR #11906: URL: https://github.com/apache/camel/pull/11906#discussion_r1382909044
########## components/camel-dynamic-router/src/main/java/org/apache/camel/component/dynamicrouter/DynamicRouterControlChannelProcessor.java: ########## @@ -156,20 +157,22 @@ DynamicRouterControlMessage handleControlMessage(final Exchange exchange) { public boolean process(final Exchange exchange, final AsyncCallback callback) { LOG.debug("Received control channel message"); DynamicRouterControlMessage controlMessage = handleControlMessage(exchange); - final DynamicRouterProcessor processor = Optional.ofNullable(component.getRoutingProcessor(controlMessage.getChannel())) - .orElseThrow(() -> new IllegalArgumentException( - "Control channel message is invalid: wrong channel, or no processors present.")); - switch (controlMessage.getMessageType()) { - case SUBSCRIBE: - processor.addFilter(controlMessage); - exchange.getIn().setBody(controlMessage.getId(), String.class); - break; - case UNSUBSCRIBE: - processor.removeFilter(controlMessage.getId()); - break; - default: - // Cannot get here due to enum - break; + try (DynamicRouterMulticastProcessor processor + = Optional.ofNullable(component.getRoutingProcessor(controlMessage.getChannel())) + .orElseThrow(() -> new IllegalArgumentException( + "Control channel message is invalid: wrong channel, or no processors present."))) { + switch (controlMessage.getMessageType()) { + case SUBSCRIBE -> { + processor.addFilter(controlMessage); + exchange.getIn().setBody(controlMessage.getId(), String.class); + } + case UNSUBSCRIBE -> processor.removeFilter(controlMessage.getId()); + default -> { + // Cannot get here due to enum + } + } + } catch (IOException e) { + throw new RuntimeException(e); Review Comment: This is wrong, you should do exchange.setException(e); ########## components/camel-dynamic-router/pom.xml: ########## @@ -69,4 +69,23 @@ <scope>test</scope> </dependency> </dependencies> + + <build> Review Comment: Is these settings really needed, we dont have this kind in the other compomnents ########## components/camel-dynamic-router/src/main/java/org/apache/camel/component/dynamicrouter/DynamicRouterControlChannelProcessor.java: ########## @@ -156,20 +157,22 @@ DynamicRouterControlMessage handleControlMessage(final Exchange exchange) { public boolean process(final Exchange exchange, final AsyncCallback callback) { LOG.debug("Received control channel message"); DynamicRouterControlMessage controlMessage = handleControlMessage(exchange); - final DynamicRouterProcessor processor = Optional.ofNullable(component.getRoutingProcessor(controlMessage.getChannel())) - .orElseThrow(() -> new IllegalArgumentException( - "Control channel message is invalid: wrong channel, or no processors present.")); - switch (controlMessage.getMessageType()) { - case SUBSCRIBE: - processor.addFilter(controlMessage); - exchange.getIn().setBody(controlMessage.getId(), String.class); - break; - case UNSUBSCRIBE: - processor.removeFilter(controlMessage.getId()); - break; - default: - // Cannot get here due to enum - break; + try (DynamicRouterMulticastProcessor processor + = Optional.ofNullable(component.getRoutingProcessor(controlMessage.getChannel())) + .orElseThrow(() -> new IllegalArgumentException( + "Control channel message is invalid: wrong channel, or no processors present."))) { Review Comment: This is wrong you should set exchange on exchange and not throw it. And then call callback done true and return true to exit ########## components/camel-dynamic-router/src/main/java/org/apache/camel/component/dynamicrouter/DynamicRouterMulticastProcessor.java: ########## @@ -0,0 +1,292 @@ +/* + * 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.dynamicrouter; + +import java.util.*; +import java.util.concurrent.ExecutorService; +import java.util.function.Supplier; +import java.util.stream.Collectors; + +import org.apache.camel.*; +import org.apache.camel.processor.FilterProcessor; +import org.apache.camel.processor.MulticastProcessor; +import org.apache.camel.processor.ProcessorExchangePair; +import org.apache.camel.spi.ProducerCache; +import org.apache.camel.support.*; +import org.apache.camel.support.service.ServiceHelper; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import static org.apache.camel.component.dynamicrouter.DynamicRouterConstants.MODE_FIRST_MATCH; + +public class DynamicRouterMulticastProcessor extends MulticastProcessor { + + private static final Logger LOG = LoggerFactory.getLogger(DynamicRouterMulticastProcessor.class); + + /** + * Template for a logging endpoint, showing all, and multiline. + */ + private static final String LOG_ENDPOINT = "log:%s.%s?level=%s&showAll=true&multiline=true"; + + private boolean ignoreInvalidEndpoints; + + private final ProducerCache producerCache; + + /** + * {@link FilterProcessor}s, mapped by subscription ID, to determine if the incoming exchange should be routed based + * on the content. + */ + private final TreeMap<String, PrioritizedFilter> filterMap; + + /** + * Indicates the behavior of the Dynamic Router when routing participants are selected to receive an incoming + * exchange. If the mode is "firstMatch", then the exchange is routed only to the first participant that has a + * matching predicate. If the mode is "allMatch", then the exchange is routed to all participants that have a + * matching predicate. + */ + private final String recipientMode; + + /** + * The {@link FilterProcessor} factory. + */ + private final Supplier<PrioritizedFilter.PrioritizedFilterFactory> filterProcessorFactorySupplier; + + /** + * Flag to log a warning if a message is dropped due to no matching filters. + */ + private final boolean warnDroppedMessage; + + public DynamicRouterMulticastProcessor(String id, CamelContext camelContext, Route route, String recipientMode, + final boolean warnDroppedMessage, + final Supplier<PrioritizedFilter.PrioritizedFilterFactory> filterProcessorFactorySupplier, + ProducerCache producerCache, AggregationStrategy aggregationStrategy, + boolean parallelProcessing, ExecutorService executorService, + boolean shutdownExecutorService, + boolean streaming, boolean stopOnException, + long timeout, Processor onPrepare, boolean shareUnitOfWork, + boolean parallelAggregate) { + super(camelContext, route, new ArrayList<>(), aggregationStrategy, parallelProcessing, executorService, + shutdownExecutorService, + streaming, stopOnException, timeout, onPrepare, + shareUnitOfWork, parallelAggregate); + setId(id); + this.producerCache = producerCache; + this.filterMap = new TreeMap<>(); + this.recipientMode = recipientMode; + this.filterProcessorFactorySupplier = filterProcessorFactorySupplier; + this.warnDroppedMessage = warnDroppedMessage; + } + + public boolean isIgnoreInvalidEndpoints() { + return ignoreInvalidEndpoints; + } + + public void setIgnoreInvalidEndpoints(boolean ignoreInvalidEndpoints) { + this.ignoreInvalidEndpoints = ignoreInvalidEndpoints; + } + + protected List<Processor> createEndpointProcessors(Exchange exchange) { + List<String> recipientList = matchFilters(exchange).stream() + .map(PrioritizedFilter::getEndpoint) + .distinct() + .map(String::trim) + .collect(Collectors.toList()); + if (recipientList.isEmpty()) { + // No matching filters, so we will use the default filter that will create a + // notification that there were no routing participants that matched the + // exchange, which results in a "dropped" message. + Message exchangeIn = exchange.getIn(); + Object originalBody = exchangeIn.getBody(); + exchangeIn.setHeader("originalBody", originalBody); + String endpoint = String.format(LOG_ENDPOINT, this.getClass().getCanonicalName(), getId(), + warnDroppedMessage ? "WARN" : "DEBUG"); + recipientList.add(endpoint); + String error = String.format( Review Comment: Hmmm why is this a plain string message returned and not an exception or something, its a bit weird ########## components/camel-dynamic-router/src/main/java/org/apache/camel/component/dynamicrouter/DynamicRouterEndpoint.java: ########## @@ -140,30 +149,87 @@ protected void doInit() throws Exception { super.doInit(); Review Comment: Is all this code in the constructor? If so it should be moved to doInit where you do all kind of initialization ########## components/camel-dynamic-router/src/main/java/org/apache/camel/component/dynamicrouter/DynamicRouterControlChannelProcessor.java: ########## @@ -156,20 +157,22 @@ DynamicRouterControlMessage handleControlMessage(final Exchange exchange) { public boolean process(final Exchange exchange, final AsyncCallback callback) { LOG.debug("Received control channel message"); DynamicRouterControlMessage controlMessage = handleControlMessage(exchange); - final DynamicRouterProcessor processor = Optional.ofNullable(component.getRoutingProcessor(controlMessage.getChannel())) - .orElseThrow(() -> new IllegalArgumentException( - "Control channel message is invalid: wrong channel, or no processors present.")); - switch (controlMessage.getMessageType()) { - case SUBSCRIBE: - processor.addFilter(controlMessage); - exchange.getIn().setBody(controlMessage.getId(), String.class); - break; - case UNSUBSCRIBE: - processor.removeFilter(controlMessage.getId()); - break; - default: - // Cannot get here due to enum - break; + try (DynamicRouterMulticastProcessor processor + = Optional.ofNullable(component.getRoutingProcessor(controlMessage.getChannel())) + .orElseThrow(() -> new IllegalArgumentException( + "Control channel message is invalid: wrong channel, or no processors present."))) { + switch (controlMessage.getMessageType()) { + case SUBSCRIBE -> { + processor.addFilter(controlMessage); + exchange.getIn().setBody(controlMessage.getId(), String.class); + } + case UNSUBSCRIBE -> processor.removeFilter(controlMessage.getId()); + default -> { + // Cannot get here due to enum + } + } + } catch (IOException e) { Review Comment: Should catch Exception not only IO ########## components/camel-dynamic-router/src/main/java/org/apache/camel/component/dynamicrouter/DynamicRouterControlChannelProcessor.java: ########## @@ -156,20 +157,22 @@ DynamicRouterControlMessage handleControlMessage(final Exchange exchange) { public boolean process(final Exchange exchange, final AsyncCallback callback) { LOG.debug("Received control channel message"); DynamicRouterControlMessage controlMessage = handleControlMessage(exchange); - final DynamicRouterProcessor processor = Optional.ofNullable(component.getRoutingProcessor(controlMessage.getChannel())) - .orElseThrow(() -> new IllegalArgumentException( - "Control channel message is invalid: wrong channel, or no processors present.")); - switch (controlMessage.getMessageType()) { - case SUBSCRIBE: - processor.addFilter(controlMessage); - exchange.getIn().setBody(controlMessage.getId(), String.class); - break; - case UNSUBSCRIBE: - processor.removeFilter(controlMessage.getId()); - break; - default: - // Cannot get here due to enum - break; + try (DynamicRouterMulticastProcessor processor + = Optional.ofNullable(component.getRoutingProcessor(controlMessage.getChannel())) + .orElseThrow(() -> new IllegalArgumentException( + "Control channel message is invalid: wrong channel, or no processors present."))) { + switch (controlMessage.getMessageType()) { + case SUBSCRIBE -> { + processor.addFilter(controlMessage); + exchange.getIn().setBody(controlMessage.getId(), String.class); Review Comment: Should maybe be changed from getIn to getMessage -- 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: commits-unsubscr...@camel.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org