[
https://issues.apache.org/jira/browse/ARTEMIS-4973?focusedWorklogId=929185&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-929185
]
ASF GitHub Bot logged work on ARTEMIS-4973:
-------------------------------------------
Author: ASF GitHub Bot
Created on: 07/Aug/24 17:19
Start Date: 07/Aug/24 17:19
Worklog Time Spent: 10m
Work Description: gemmellr commented on code in PR #5128:
URL: https://github.com/apache/activemq-artemis/pull/5128#discussion_r1707484372
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/paging/impl/PagingStoreImpl.java:
##########
@@ -228,11 +228,58 @@ private void configureSizeMetric() {
size.setMax(maxSize, maxSize, maxMessages, maxMessages);
}
+ private boolean validateNewSettings(final AddressSettings addressSettings) {
+
+ Long newPageLimitBytes = addressSettings.getPageLimitBytes();
+
+ if (newPageLimitBytes != null && newPageLimitBytes < 0) {
+ newPageLimitBytes = null;
+ }
+
+ int newPageSize =
storageManager.getAllowedPageSize(addressSettings.getPageSizeBytes());
+
+ if (newPageLimitBytes != null && newPageSize > 0) {
+
+ if (newPageSize > newPageLimitBytes) {
+ ActiveMQServerLogger.LOGGER.pageSettingsFailedApply(storeName,
addressSettings, "pageLimitBytes is smaller than pageSize. It should allow at
least one page file");
+ return false;
+ }
Review Comment:
I think this should probably just throw personally.
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/paging/impl/PagingStoreImpl.java:
##########
@@ -228,11 +228,58 @@ private void configureSizeMetric() {
size.setMax(maxSize, maxSize, maxMessages, maxMessages);
}
+ private boolean validateNewSettings(final AddressSettings addressSettings) {
+
+ Long newPageLimitBytes = addressSettings.getPageLimitBytes();
+
+ if (newPageLimitBytes != null && newPageLimitBytes < 0) {
+ newPageLimitBytes = null;
+ }
+
+ int newPageSize =
storageManager.getAllowedPageSize(addressSettings.getPageSizeBytes());
+
+ if (newPageLimitBytes != null && newPageSize > 0) {
+
+ if (newPageSize > newPageLimitBytes) {
+ ActiveMQServerLogger.LOGGER.pageSettingsFailedApply(storeName,
addressSettings, "pageLimitBytes is smaller than pageSize. It should allow at
least one page file");
+ return false;
+ }
+
+ long newEstimatedMaxPages = newPageLimitBytes / newPageSize;
+
+ long existingNumberOfPages;
+ if (!running) {
+ try {
+ checkFileFactory();
+ List<String> files = this.fileFactory.listFiles("page");
+ existingNumberOfPages = files.size();
+ } catch (Exception e) {
+ ActiveMQServerLogger.LOGGER.pageSettingsFailedApply(storeName,
addressSettings, "failed to get existing page files with exception: " + e);
+ return false;
+ }
+ } else {
+ existingNumberOfPages = this.numberOfPages;
+ }
+
+ if (existingNumberOfPages > newEstimatedMaxPages) {
+ ActiveMQServerLogger.LOGGER.pageSettingsFailedApply(storeName,
addressSettings, "estimated max pages " + newEstimatedMaxPages + "
(pageLimitBytes / pageSize) is less than current number of pages " +
existingNumberOfPages);
+ return false;
+ }
Review Comment:
Per my other comments, I now question this entirely (not the log message,
but the return false). If someone wants to set a limit, that is lower than the
current usage, its not clear to me that is a reason not to set it. They can
consume those thigns to get back below it, and untilt hen it would act as if at
the limit and stop more being added, which is as close to the desired behaviour
as its possible to get. Failing to set anything might just mean usage keeps
increasing beyond what it already is, i.e the exact opposite of what they want
to happen.
##########
tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/paging/PagingSendTest.java:
##########
@@ -288,6 +291,103 @@ public void
testPagingDoesNotDuplicateBatchMessagesAfterPagingStarted() throws E
}
}
+ @Test
+ public void testPageLimitBytesValidation() throws Exception {
+
+ try (ClientSessionFactory sf = createSessionFactory(locator)) {
+ ClientSession session = sf.createSession(false, false);
+
+ SimpleString queueAddr = SimpleString.of("FOO");
+ session.createQueue(QueueConfiguration.of(queueAddr));
+
+ int size = 1048576;
+ AddressSettings addressSettings = new AddressSettings();
+ addressSettings.setPageFullMessagePolicy(PageFullMessagePolicy.FAIL);
+ addressSettings.setPageSizeBytes(size);
+ addressSettings.setPageLimitBytes(Long.valueOf(size));
+ addressSettings.setMaxSizeBytes(size);
Review Comment:
Setting the address full policy to PAGE explicitly would make this more
understandable, and less brittle should the default change
##########
tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/paging/PagingSendTest.java:
##########
@@ -288,6 +291,103 @@ public void
testPagingDoesNotDuplicateBatchMessagesAfterPagingStarted() throws E
}
}
+ @Test
+ public void testPageLimitBytesValidation() throws Exception {
+
+ try (ClientSessionFactory sf = createSessionFactory(locator)) {
+ ClientSession session = sf.createSession(false, false);
+
+ SimpleString queueAddr = SimpleString.of("FOO");
+ session.createQueue(QueueConfiguration.of(queueAddr));
+
+ int size = 1048576;
+ AddressSettings addressSettings = new AddressSettings();
+ addressSettings.setPageFullMessagePolicy(PageFullMessagePolicy.FAIL);
+ addressSettings.setPageSizeBytes(size);
+ addressSettings.setPageLimitBytes(Long.valueOf(size));
+ addressSettings.setMaxSizeBytes(size);
+
+ server.getAddressSettingsRepository().addMatch("FOO",
addressSettings);
+
+ int totalMessages = 15;
+ int messageSize = 90000;
+ sendMessageBatch(totalMessages, messageSize, session, queueAddr);
+
+ Queue queue = server.locateQueue(queueAddr);
+
+ // Give time Queue.deliverAsync to deliver messages
+ assertTrue(waitForMessages(queue, totalMessages, 10000));
+
+ PagingStore queuePagingStore = queue.getPagingStore();
+ assertTrue(queuePagingStore != null && queuePagingStore.isPaging());
+
+ // set page size bytes to be larger than pageLimitBytes
+ addressSettings.setPageSizeBytes(size * 2);
+ server.getAddressSettingsRepository().addMatch("FOO",
addressSettings);
+
+ // check the original pageSizeBytes is not changed
+ assertEquals(size, queuePagingStore.getPageSizeBytes());
+
+ // send a messages should be allowed because the page file still have
space
+ sendMessageBatch(1, messageSize, session, queueAddr);
+ assertTrue(waitForMessages(queue, totalMessages + 1, 10000));
+ }
+ }
+
+ @Test
+ public void testPageLimitBytesValidationOnRestart() throws Exception {
+
+ try (ClientSessionFactory sf = createSessionFactory(locator)) {
+ ClientSession session = sf.createSession(false, false);
+
+ SimpleString queueAddr = SimpleString.of("FOO");
+ session.createQueue(QueueConfiguration.of(queueAddr));
+
+ int size = 1024 * 50;
+ AddressSettings addressSettings = new AddressSettings();
+ addressSettings.setPageFullMessagePolicy(PageFullMessagePolicy.FAIL);
+ addressSettings.setPageSizeBytes(size);
+ addressSettings.setPageLimitBytes(Long.valueOf(size * 10));
+ addressSettings.setMaxSizeBytes(size);
+
+ server.getAddressSettingsRepository().addMatch("FOO",
addressSettings);
+
+ int totalMessages = 30;
+ int messageSize = 1024 * 10;
+ sendMessageBatch(totalMessages, messageSize, session, queueAddr);
+
+ Queue queue = server.locateQueue(queueAddr);
+
+ // Give time Queue.deliverAsync to deliver messages
+ assertTrue(waitForMessages(queue, totalMessages, 10000));
+
+ PagingStore queuePagingStore = queue.getPagingStore();
+ assertTrue(queuePagingStore != null && queuePagingStore.isPaging());
+
+ long existingPages = queuePagingStore.getNumberOfPages();
+ assertTrue(existingPages > 4);
+
+ // restart the server and the invalid settings are not applied too.
+ server.stop(true);
+ waitForServerToStop(server);
+
+ addressSettings.setPageLimitBytes(Long.valueOf(size * 4));
+ server.getAddressSettingsRepository().addMatch("FOO",
addressSettings);
+
+ server.start();
+ waitForServerToStart(server);
+
+ queue = server.locateQueue(queueAddr);
+ queuePagingStore = queue.getPagingStore();
+
+ // check settings not applied
+ assertEquals(0, queuePagingStore.getPageSizeBytes());
+ assertNull(queuePagingStore.getPageFullMessagePolicy());
+ assertNull(queuePagingStore.getPageLimitBytes());
+ assertEquals(0, queuePagingStore.getMaxSize());
Review Comment:
Its not clear to me what the behavioural state of the address would actually
be in this case? Will it allow more paging? Seems like a test should validate
the expected behaviour rather than just the return here. If it does allow more
paging as it seems it might, then that seems questionable since a limit was
attempted to be imposed, and the reason it failed is seemingly because it was
already beyond it? That seems off. Why not allow it but log it is beyond limit
already, rather than reject it and then permit _yet more_ paging beyond the
desired limit as a result?
Issue Time Tracking
-------------------
Worklog Id: (was: 929185)
Time Spent: 2h 40m (was: 2.5h)
> pageSizeBytes/pageLimitBytes combination can cause Address full
> ---------------------------------------------------------------
>
> Key: ARTEMIS-4973
> URL: https://issues.apache.org/jira/browse/ARTEMIS-4973
> Project: ActiveMQ Artemis
> Issue Type: Bug
> Components: Broker
> Affects Versions: 2.36.0
> Reporter: Howard Gao
> Assignee: Howard Gao
> Priority: Major
> Time Spent: 2h 40m
> Remaining Estimate: 0h
>
> There is an edge case where adjusting pageSizeBytes can cause "Address is
> full" errors, even though the address is not full.
> Do we need to enforce that pageSizeBytes <= pageLimitBytes?
> Reproducer steps:
> Step 1: configure pageSizeBytes == pageLimitBytes == 1mb:
> $ cat my.broker.properties
> addressSettings."FOO".pageSizeBytes=1048576
> addressSettings."FOO".pageLimitBytes=1048576
> addressSettings."FOO".maxSizeBytes=1048576
> addressSettings."FOO".pageFullMessagePolicy=FAIL
> addressConfigurations."FOO".routingTypes=MULTICAST
> addressConfigurations."FOO".queueConfigs."FOO".name=FOO
> addressConfigurations."FOO".queueConfigs."FOO".address=FOO
> addressConfigurations."FOO".queueConfigs."FOO".routingType=MULTICAST
> Step 2: run broker
> bin/artemis run --properties my.broker.properties
> Step 3: produce 15 messages
> $ bin/artemis producer --user admin --password admin --destination
> topic://FOO --message-count 15 --message-size 100000 --protocol amqp
> Step 4: observe paging started on the destination (but the page size is
> 328kb, can hold more messages)
> INFO [org.apache.activemq.artemis.core.server] AMQ222038: Starting paging on
> address 'FOO'; size=1107003 bytes (11 messages); maxSize=1048576 bytes (-1
> messages); globalSize=1107003 bytes (11 messages); globalMaxSize=1073741824
> bytes (-1 messages);
> Step 5: stop broker, increase page size
> cat my.broker.properties
> addressSettings."FOO".pageSizeBytes=4048576
> ...
> Step 6: run broker, observe logs show paging warning
> 2024-06-25 15:23:47,135 WARN [org.apache.activemq.artemis.core.server]
> AMQ224123: Address FOO has more pages than allowed. System currently has 1
> pages, while the estimated max number of pages is 0 based on the
> page-limit-bytes (1048576) / page-size (4048576)
> Step 7: try to produce a message, address full
> WARN [org.apache.activemq.artemis.protocol.amqp.broker.AMQPSessionCallback]
> AMQ229102: Address "FOO" is full.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@activemq.apache.org
For additional commands, e-mail: issues-h...@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
