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?
--
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]
For further information, visit: https://activemq.apache.org/contact
