gemmellr commented on code in PR #5160: URL: https://github.com/apache/activemq-artemis/pull/5160#discussion_r1722970709
########## tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/amqp/AMQPBlockingTest.java: ########## @@ -0,0 +1,128 @@ +/** + * 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 + * <p> + * http://www.apache.org/licenses/LICENSE-2.0 + * <p> + * 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.activemq.artemis.tests.integration.amqp; + +import org.apache.activemq.artemis.core.server.ActiveMQServer; +import org.apache.activemq.artemis.core.server.files.FileStoreMonitor; +import org.apache.activemq.artemis.core.server.impl.ActiveMQServerImpl; +import org.apache.activemq.artemis.core.settings.impl.AddressFullMessagePolicy; +import org.apache.activemq.artemis.core.settings.impl.AddressSettings; +import org.apache.activemq.transport.amqp.client.*; +import org.junit.jupiter.api.Test; + +import java.net.URI; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; + +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; + +public class AMQPBlockingTest extends GlobalDiskFullTest { Review Comment: This test class name seems rather generic for what its testing and how. It is testing handling of 'FAIL' policy behaviour while the disk is full/not, and reusing the GlobalDiskFullTest tests in part to do it. Something like GlobalDiskFullFailPolicyTest to link the two might be nicer. EDIT: actually, it is overriding (without indicating so) the only test method in GlobalDiskFullTest, so it doesnt seem worth even extending that class in the current state. They are still testing related things however so naming them accordingly would still make sense. ########## tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/amqp/AMQPBlockingTest.java: ########## @@ -0,0 +1,128 @@ +/** + * 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 + * <p> + * http://www.apache.org/licenses/LICENSE-2.0 + * <p> + * 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.activemq.artemis.tests.integration.amqp; + +import org.apache.activemq.artemis.core.server.ActiveMQServer; +import org.apache.activemq.artemis.core.server.files.FileStoreMonitor; +import org.apache.activemq.artemis.core.server.impl.ActiveMQServerImpl; +import org.apache.activemq.artemis.core.settings.impl.AddressFullMessagePolicy; +import org.apache.activemq.artemis.core.settings.impl.AddressSettings; +import org.apache.activemq.transport.amqp.client.*; +import org.junit.jupiter.api.Test; + +import java.net.URI; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; + +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; + +public class AMQPBlockingTest extends GlobalDiskFullTest { + + @Override + protected ActiveMQServer createServer(int port) throws Exception { + ActiveMQServer server = this.createServer(true, true); + server.getConfiguration().getAcceptorConfigurations().clear(); + server.getConfiguration().getAcceptorConfigurations().add(this.addAcceptorConfiguration(server, port)); + server.getConfiguration().setName("localhost"); + server.getConfiguration().setJournalDirectory(server.getConfiguration().getJournalDirectory() + port); + server.getConfiguration().setBindingsDirectory(server.getConfiguration().getBindingsDirectory() + port); + server.getConfiguration().setPagingDirectory(server.getConfiguration().getPagingDirectory() + port); + server.getConfiguration().setMessageExpiryScanPeriod(5000L); + AddressSettings addressSettings = new AddressSettings(); + addressSettings.setAddressFullMessagePolicy(AddressFullMessagePolicy.FAIL); + server.getConfiguration().addAddressSetting("TEST",addressSettings); + this.addAdditionalAcceptors(server); + this.configureAddressPolicy(server); + this.configureBrokerSecurity(server); + this.addConfiguration(server); + server.start(); + this.createAddressAndQueues(server); + return server; + } Review Comment: Given that GlobalDiskFullTest manages not to override this method at all, and the ancestor parent AmqpTestSupport already sets many of these options to these same values, and offers specific methods for tweaking the config / address settings / etc to the tests needs, I'd expect something far simpler and cleaner than this can/should be done here. ########## tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/amqp/AMQPBlockingTest.java: ########## @@ -0,0 +1,128 @@ +/** + * 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 + * <p> + * http://www.apache.org/licenses/LICENSE-2.0 + * <p> + * 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. + */ Review Comment: Should be a comment not javadoc, and be indented properly (e.g the URL). See other tests (e.g GlobalDiskFullTest this currently subclasses) ########## tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/amqp/AMQPBlockingTest.java: ########## @@ -0,0 +1,128 @@ +/** + * 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 + * <p> + * http://www.apache.org/licenses/LICENSE-2.0 + * <p> + * 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.activemq.artemis.tests.integration.amqp; + +import org.apache.activemq.artemis.core.server.ActiveMQServer; +import org.apache.activemq.artemis.core.server.files.FileStoreMonitor; +import org.apache.activemq.artemis.core.server.impl.ActiveMQServerImpl; +import org.apache.activemq.artemis.core.settings.impl.AddressFullMessagePolicy; +import org.apache.activemq.artemis.core.settings.impl.AddressSettings; +import org.apache.activemq.transport.amqp.client.*; +import org.junit.jupiter.api.Test; + +import java.net.URI; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; + +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; + +public class AMQPBlockingTest extends GlobalDiskFullTest { + + @Override + protected ActiveMQServer createServer(int port) throws Exception { + ActiveMQServer server = this.createServer(true, true); + server.getConfiguration().getAcceptorConfigurations().clear(); + server.getConfiguration().getAcceptorConfigurations().add(this.addAcceptorConfiguration(server, port)); + server.getConfiguration().setName("localhost"); + server.getConfiguration().setJournalDirectory(server.getConfiguration().getJournalDirectory() + port); + server.getConfiguration().setBindingsDirectory(server.getConfiguration().getBindingsDirectory() + port); + server.getConfiguration().setPagingDirectory(server.getConfiguration().getPagingDirectory() + port); + server.getConfiguration().setMessageExpiryScanPeriod(5000L); + AddressSettings addressSettings = new AddressSettings(); + addressSettings.setAddressFullMessagePolicy(AddressFullMessagePolicy.FAIL); Review Comment: The Jira indicates an issue with either policy of either FAIL or DROP...but the code change seems like it could only affect FAIL, because its behind a gate checking for FAIL? Is another test and possibly fix needed? The test method code itself below for FAIL looks essentially identical to the existing GlobalDiskFullTest, which I presume is covering PAGE, so if the same works for DROP then perhaps they could all share the same overall base test, setting the specific mode covered via the constructor (either via ParameterizedTestExtension usage, or actual trivial subclasses of a base class) ########## tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/amqp/AMQPBlockingTest.java: ########## @@ -0,0 +1,128 @@ +/** + * 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 + * <p> + * http://www.apache.org/licenses/LICENSE-2.0 + * <p> + * 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.activemq.artemis.tests.integration.amqp; + +import org.apache.activemq.artemis.core.server.ActiveMQServer; +import org.apache.activemq.artemis.core.server.files.FileStoreMonitor; +import org.apache.activemq.artemis.core.server.impl.ActiveMQServerImpl; +import org.apache.activemq.artemis.core.settings.impl.AddressFullMessagePolicy; +import org.apache.activemq.artemis.core.settings.impl.AddressSettings; +import org.apache.activemq.transport.amqp.client.*; +import org.junit.jupiter.api.Test; + +import java.net.URI; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; + +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; + +public class AMQPBlockingTest extends GlobalDiskFullTest { + + @Override + protected ActiveMQServer createServer(int port) throws Exception { + ActiveMQServer server = this.createServer(true, true); + server.getConfiguration().getAcceptorConfigurations().clear(); + server.getConfiguration().getAcceptorConfigurations().add(this.addAcceptorConfiguration(server, port)); + server.getConfiguration().setName("localhost"); + server.getConfiguration().setJournalDirectory(server.getConfiguration().getJournalDirectory() + port); + server.getConfiguration().setBindingsDirectory(server.getConfiguration().getBindingsDirectory() + port); + server.getConfiguration().setPagingDirectory(server.getConfiguration().getPagingDirectory() + port); + server.getConfiguration().setMessageExpiryScanPeriod(5000L); + AddressSettings addressSettings = new AddressSettings(); + addressSettings.setAddressFullMessagePolicy(AddressFullMessagePolicy.FAIL); + server.getConfiguration().addAddressSetting("TEST",addressSettings); + this.addAdditionalAcceptors(server); + this.configureAddressPolicy(server); + this.configureBrokerSecurity(server); + this.addConfiguration(server); + server.start(); + this.createAddressAndQueues(server); + return server; + } + + @Test + public void testProducerOnDiskFull() throws Exception { + FileStoreMonitor monitor = ((ActiveMQServerImpl)server).getMonitor().setMaxUsage(0.0); + final CountDownLatch latch = new CountDownLatch(1); + monitor.addCallback((usableSpace, totalSpace, ok, type) -> { + latch.countDown(); + }); + + assertTrue(latch.await(1, TimeUnit.MINUTES)); + + AmqpClient client = createAmqpClient(new URI("tcp://localhost:" + AMQP_PORT)); + AmqpConnection connection = addConnection(client.connect()); + + try { + AmqpSession session = connection.createSession(); + AmqpSender sender = session.createSender("TEST"); + byte[] payload = new byte[1000]; + + AmqpSender anonSender = session.createSender(); + + CountDownLatch sentWithName = new CountDownLatch(1); + CountDownLatch sentAnon = new CountDownLatch(1); + + Thread threadWithName = new Thread(() -> { + try { + final AmqpMessage message = new AmqpMessage(); + message.setBytes(payload); + sender.setSendTimeout(-1); + sender.send(message); + } catch (Exception e) { + e.printStackTrace(); + } finally { + sentWithName.countDown(); + } + }); + + threadWithName.start(); + + + Thread threadWithAnon = new Thread(() -> { + try { + final AmqpMessage message = new AmqpMessage(); + message.setBytes(payload); + anonSender.setSendTimeout(-1); + message.setAddress(getQueueName()); + anonSender.send(message); + } catch (Exception e) { + e.printStackTrace(); + } finally { + sentAnon.countDown(); + } + }); + + threadWithAnon.start(); Review Comment: Create an Executor to use, will be cleaner and the the test can also ensure it is e.g shutdownNow at the end by using the runAfter(..) mechanism (various other tests that do this). ########## tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/amqp/AMQPBlockingTest.java: ########## @@ -0,0 +1,128 @@ +/** + * 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 + * <p> + * http://www.apache.org/licenses/LICENSE-2.0 + * <p> + * 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.activemq.artemis.tests.integration.amqp; + +import org.apache.activemq.artemis.core.server.ActiveMQServer; +import org.apache.activemq.artemis.core.server.files.FileStoreMonitor; +import org.apache.activemq.artemis.core.server.impl.ActiveMQServerImpl; +import org.apache.activemq.artemis.core.settings.impl.AddressFullMessagePolicy; +import org.apache.activemq.artemis.core.settings.impl.AddressSettings; +import org.apache.activemq.transport.amqp.client.*; Review Comment: Will need to lose the star import, it will cause a checkstyle failure. -- 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: gitbox-unsubscr...@activemq.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: gitbox-unsubscr...@activemq.apache.org For additional commands, e-mail: gitbox-h...@activemq.apache.org For further information, visit: https://activemq.apache.org/contact
