chibenwa commented on code in PR #3194: URL: https://github.com/apache/james-project/pull/3194#discussion_r4069149732
########## server/queue/queue-activemq/src/main/java/org/apache/james/queue/activemq/ActiveMQMailQueueFactory.java: ########## @@ -1,64 +1,58 @@ -/**************************************************************** - * 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.james.queue.activemq; - -import jakarta.inject.Inject; -import jakarta.jms.ConnectionFactory; - -import org.apache.james.metrics.api.GaugeRegistry; -import org.apache.james.metrics.api.MetricFactory; -import org.apache.james.queue.activemq.metric.ActiveMQMetricCollector; -import org.apache.james.queue.api.MailQueueFactory; -import org.apache.james.queue.api.MailQueueItemDecoratorFactory; -import org.apache.james.queue.api.MailQueueName; -import org.apache.james.queue.api.ManageableMailQueue; -import org.apache.james.queue.jms.JMSMailQueueFactory; - -/** - * {@link MailQueueFactory} implementations which return - * {@link ActiveMQCacheableMailQueue} instances - */ -public class ActiveMQMailQueueFactory extends JMSMailQueueFactory { - - private boolean useBlob = true; - - private final ActiveMQMetricCollector activeMQMetricCollector; - - public ActiveMQMailQueueFactory(ConnectionFactory connectionFactory, MailQueueItemDecoratorFactory mailQueueItemDecoratorFactory, MetricFactory metricFactory, - GaugeRegistry gaugeRegistry, ActiveMQMetricCollector activeMQMetricCollector) { - super(connectionFactory, mailQueueItemDecoratorFactory, metricFactory, gaugeRegistry); - this.activeMQMetricCollector = activeMQMetricCollector; - } - - @Inject - public ActiveMQMailQueueFactory(EmbeddedActiveMQ embeddedActiveMQ, MailQueueItemDecoratorFactory mailQueueItemDecoratorFactory, MetricFactory metricFactory, - GaugeRegistry gaugeRegistry, ActiveMQMetricCollector activeMQMetricCollector) { - this(embeddedActiveMQ.getConnectionFactory(), mailQueueItemDecoratorFactory, metricFactory, gaugeRegistry, activeMQMetricCollector); - } - - public void setUseBlobMessages(boolean useBlob) { - this.useBlob = useBlob; - } - - @Override - protected ManageableMailQueue createCacheableMailQueue(MailQueueName name) { - activeMQMetricCollector.collectQueueStatistics(name); - return new ActiveMQCacheableMailQueue(connectionFactory, mailQueueItemDecoratorFactory, name, useBlob, metricFactory, gaugeRegistry); - } -} +/**************************************************************** + * 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.james.queue.activemq; + +import jakarta.inject.Inject; +import jakarta.jms.ConnectionFactory; + +import org.apache.james.metrics.api.GaugeRegistry; +import org.apache.james.metrics.api.MetricFactory; +import org.apache.james.queue.activemq.metric.ActiveMQMetricCollector; +import org.apache.james.queue.api.MailQueueFactory; +import org.apache.james.queue.api.MailQueueItemDecoratorFactory; +import org.apache.james.queue.api.MailQueueName; +import org.apache.james.queue.api.ManageableMailQueue; +import org.apache.james.queue.jms.JMSMailQueueFactory; + +/** + * {@link MailQueueFactory} implementation which returns + * {@link ActiveMQCacheableMailQueue} instances backed by Apache ActiveMQ Artemis. + */ +public class ActiveMQMailQueueFactory extends JMSMailQueueFactory { Review Comment: ActiveMQCacheableMailQueue and ActiveMQMailQueueFactory adds no specific value compared to the JMS base implem. Correct ? Then why not just use JMS ? ########## server/queue/queue-activemq/src/main/java/org/apache/james/queue/activemq/EmbeddedActiveMQ.java: ########## @@ -23,107 +23,76 @@ import jakarta.inject.Inject; import jakarta.jms.ConnectionFactory; -import org.apache.activemq.ActiveMQConnectionFactory; -import org.apache.activemq.ActiveMQPrefetchPolicy; -import org.apache.activemq.blob.BlobTransferPolicy; -import org.apache.activemq.broker.BrokerPlugin; -import org.apache.activemq.broker.BrokerService; -import org.apache.activemq.broker.jmx.ManagementContext; -import org.apache.activemq.plugin.StatisticsBrokerPlugin; -import org.apache.activemq.store.PersistenceAdapter; +import org.apache.activemq.artemis.api.core.TransportConfiguration; +import org.apache.activemq.artemis.core.config.Configuration; +import org.apache.activemq.artemis.core.config.impl.ConfigurationImpl; +import org.apache.activemq.artemis.core.remoting.impl.invm.InVMAcceptorFactory; +import org.apache.activemq.artemis.jms.client.ActiveMQConnectionFactory; import org.apache.james.filesystem.api.FileSystem; import org.slf4j.Logger; import org.slf4j.LoggerFactory; +/** + * Embedded Artemis broker replacing the legacy ActiveMQ embedded broker. + * Uses Apache ActiveMQ Artemis (Jakarta JMS) as the underlying message broker. + */ public class EmbeddedActiveMQ { private static final Logger LOGGER = LoggerFactory.getLogger(EmbeddedActiveMQ.class); - private static final String KAHADB_STORE_LOCATION = "file://var/store/activemq/brokers/KahaDB"; - private static final String BLOB_TRANSFER_LOCATION = "file://var/store/activemq/blob-transfer"; - private static final String BROCKERS_LOCATION = "file://var/store/activemq/brokers"; - private static final String BROKER_ID = "broker"; + private static final String DATA_DIRECTORY_RELATIVE = "var/store/artemis"; private static final String BROKER_NAME = "james"; - private static final String BROCKER_URI = "tcp://localhost:0"; - private static final String STORE_USAGE_LIMIT_PROPERTY = "james.activemq.store.usage.limit.bytes"; - private static final String TEMP_USAGE_LIMIT_PROPERTY = "james.activemq.temp.usage.limit.bytes"; - private static final long DEFAULT_STORE_USAGE_LIMIT_BYTES = 10L * 1024 * 1024 * 1024; // 10 GB - private static final long DEFAULT_TEMP_USAGE_LIMIT_BYTES = 5L * 1024 * 1024 * 1024; // 5 GB - private final ActiveMQConnectionFactory activeMQConnectionFactory; - private final PersistenceAdapter persistenceAdapter; - private BrokerService brokerService; + private final ActiveMQConnectionFactory connectionFactory; + private final org.apache.activemq.artemis.core.server.embedded.EmbeddedActiveMQ embeddedServer; @Inject - private EmbeddedActiveMQ(FileSystem fileSystem, PersistenceAdapter persistenceAdapter, ActiveMQConfiguration configuration) { - this.persistenceAdapter = persistenceAdapter; + public EmbeddedActiveMQ(FileSystem fileSystem, ActiveMQConfiguration configuration) { try { - persistenceAdapter.setDirectory(fileSystem.getFile(KAHADB_STORE_LOCATION)); - launchEmbeddedBroker(fileSystem, configuration); + String dataDirectory = fileSystem.getFile("file://" + DATA_DIRECTORY_RELATIVE).getAbsolutePath(); + embeddedServer = createAndStartBroker(dataDirectory); + connectionFactory = createConnectionFactory(); } catch (Exception e) { - throw new RuntimeException(e); + throw new RuntimeException("Failed to start embedded Artemis broker", e); } - activeMQConnectionFactory = createActiveMQConnectionFactory(createBlobTransferPolicy(fileSystem)); } public ConnectionFactory getConnectionFactory() { - return activeMQConnectionFactory; + return connectionFactory; } @PreDestroy public void stop() throws Exception { - LOGGER.info("Stopping embedded ActiveMQ..."); - brokerService.stop(); - LOGGER.info("Stopped embedded ActiveMQ"); + LOGGER.info("Stopping embedded Artemis broker..."); + embeddedServer.stop(); + connectionFactory.close(); + LOGGER.info("Stopped embedded Artemis broker"); } - private ActiveMQConnectionFactory createActiveMQConnectionFactory(BlobTransferPolicy blobTransferPolicy) { - ActiveMQConnectionFactory connectionFactory = new ActiveMQConnectionFactory("vm://james?create=false"); - connectionFactory.setTrustAllPackages(false); - connectionFactory.setBlobTransferPolicy(blobTransferPolicy); - connectionFactory.setPrefetchPolicy(createActiveMQPrefetchPolicy()); - return connectionFactory; - } - - private ActiveMQPrefetchPolicy createActiveMQPrefetchPolicy() { - ActiveMQPrefetchPolicy prefetchPolicy = new ActiveMQPrefetchPolicy(); - prefetchPolicy.setQueuePrefetch(0); - prefetchPolicy.setTopicPrefetch(0); - return prefetchPolicy; - } + private org.apache.activemq.artemis.core.server.embedded.EmbeddedActiveMQ createAndStartBroker(String dataDirectory) throws Exception { + Configuration config = new ConfigurationImpl() + .setSecurityEnabled(false) + .setJMXManagementEnabled(false) + .setPersistenceEnabled(true) + .setJournalDirectory(dataDirectory + "/journal") + .setBindingsDirectory(dataDirectory + "/bindings") + .setLargeMessagesDirectory(dataDirectory + "/largemessages") Review Comment: <3 nice we do not have to implement this ourselves ########## ARTEMIS_MIGRATION.md: ########## @@ -0,0 +1,157 @@ +# Миграция Apache James с ActiveMQ (Classic) на Apache ActiveMQ Artemis + +В данном документе подробно описаны архитектурные и кодовые изменения, выполненные в проекте James (`C:\soft\james_src\james-project-fast`) для замены встроенного брокера сообщений **Apache ActiveMQ (Classic 6.x)** на **Apache ActiveMQ Artemis (2.56.0)**. Review Comment: English please ########## server/queue/queue-activemq/src/test/java/org/apache/james/queue/activemq/ActiveMQMailQueueBlobTest.java: ########## @@ -52,35 +52,31 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; +/** + * Test for the Artemis-backed ActiveMQCacheableMailQueue. + * Blob functionality is not available with Artemis and is not tested here. + */ Review Comment: Remove? ########## server/queue/queue-activemq/src/test/java/org/apache/james/queue/activemq/metric/ActiveMQMetricCollectorTest.java: ########## @@ -19,126 +19,47 @@ package org.apache.james.queue.activemq.metric; -import static org.assertj.core.api.Assertions.assertThat; -import static org.assertj.core.api.Assertions.assertThatThrownBy; - -import java.time.Duration; -import java.util.Map; -import java.util.concurrent.ConcurrentHashMap; - -import jakarta.jms.JMSException; - -import org.apache.activemq.ActiveMQConnectionFactory; -import org.apache.activemq.ActiveMQPrefetchPolicy; -import org.apache.activemq.broker.BrokerService; -import org.apache.james.metrics.api.Gauge; -import org.apache.james.metrics.api.GaugeRegistry; -import org.apache.james.metrics.api.NoopGaugeRegistry; -import org.apache.james.metrics.tests.RecordingMetricFactory; -import org.apache.james.queue.activemq.ActiveMQConfiguration; -import org.apache.james.queue.api.MailQueueName; import org.apache.james.queue.jms.BrokerExtension; -import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.Disabled; import org.junit.jupiter.api.Tag; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; -import reactor.core.publisher.Flux; -import reactor.core.publisher.Mono; - +/** + * Tests for ActiveMQMetricCollectorImpl which rely on the ActiveMQ Statistics Plugin. + * + * This plugin is specific to legacy Apache ActiveMQ and is not available in + * Apache ActiveMQ Artemis. The broker has been migrated to Artemis, and the + * metric collection via the Statistics Plugin is now disabled (using {@link ActiveMQMetricCollectorNoop}). + * + * Artemis provides queue statistics via JMX and its management API. + * These tests are kept for reference but disabled until Artemis-specific metric + * collection is implemented. + */ @ExtendWith(BrokerExtension.class) @Tag(BrokerExtension.STATISTICS) +@Disabled("ActiveMQ Statistics Plugin is not available in Artemis broker. " + + "Metrics are now disabled (ActiveMQMetricCollectorNoop). " + + "Implement Artemis-specific metric collection to re-enable.") class ActiveMQMetricCollectorTest { - private static ActiveMQConnectionFactory connectionFactory; - private static final ActiveMQConfiguration EMPTY_CONFIGURATION = ActiveMQConfiguration.getDefault(); - - @BeforeAll - static void setup(BrokerService broker) { - connectionFactory = new ActiveMQConnectionFactory("vm://localhost?create=false"); - ActiveMQPrefetchPolicy prefetchPolicy = new ActiveMQPrefetchPolicy(); - prefetchPolicy.setQueuePrefetch(0); - connectionFactory.setPrefetchPolicy(prefetchPolicy); - } - @Test void shouldFailToFetchAndUpdateStatisticsForUnknownQueue() { - SimpleGaugeRegistry gaugeRegistry = new SimpleGaugeRegistry(); - ActiveMQMetricCollectorImpl testee = new ActiveMQMetricCollectorImpl(EMPTY_CONFIGURATION, connectionFactory, new RecordingMetricFactory(), gaugeRegistry); - ActiveMQMetrics queueStatistics = ActiveMQMetrics.forQueue("UNKNOWN", gaugeRegistry); - - assertThatThrownBy(() -> testee.fetchAndUpdate(queueStatistics)) - .isInstanceOf(JMSException.class); - - assertThat(gaugeRegistry.getGauge("ActiveMQ.Statistics.Destination.UNKNOWN")).isNull(); + // disabled - see class-level @Disabled Review Comment: Remove those disabled tests ? ########## ARTEMIS_MIGRATION_EN.md: ########## @@ -0,0 +1,156 @@ +# Migration Guide: Apache James ActiveMQ (Classic) to Apache ActiveMQ Artemis Review Comment: This file is very developer oriented and looks like LLM instructions. I would recommend to strip it. or convert it to `src/adr` format. Also have a (brief) mention of this in upgrade-instructions.md from an operator perpective (upgrading to higher versions will loose you mailqueue so upgrade with empty mail queue , which can be achieved by isolating the mail server, and flush mail queues). -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
