gemmellr commented on code in PR #5128:
URL: https://github.com/apache/activemq-artemis/pull/5128#discussion_r1717017555
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/paging/impl/PagingStoreImpl.java:
##########
@@ -289,9 +290,23 @@ public void applySetting(final AddressSettings
addressSettings) {
this.pageLimitBytes = null;
}
+ this.pageLimitMessages = addressSettings.getPageLimitMessages();
Review Comment:
This is in the wrong place, it should be _before_ the matching 'null
replacement check' for the value, up on line 273, i.e where it was to begin
with (originally as line 271), otherwise it may think the value has changed
when it hasnt (but the comparison fouled because the null replacement has been
'bypassed' here).
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/paging/impl/PagingStoreImpl.java:
##########
@@ -289,9 +290,23 @@ public void applySetting(final AddressSettings
addressSettings) {
this.pageLimitBytes = null;
}
+ this.pageLimitMessages = addressSettings.getPageLimitMessages();
+ if (firstTime || !Objects.equals(this.pageLimitMessages,
originalLimitMessages)) {
+ if (this.cursorProvider != null) {
+ this.cursorProvider.checkClearPageLimit();
+ }
+ }
Review Comment:
The cursorProvider hasnt been created at the point this is called during the
constructor, so its never going to be non-null if 'firstTime' is true and it
cant be non-null in real usage if it is false. My suggestion on the prior round
was actually that it should _not_ do this for the constructor, since this is
handled via the start etc processes later after construction and so would
basically be duplicate.
This is also going to check with the _previous_ estimated max pages rather
than any new value, because that value hasnt been updated yet (its done later
below). I dont think that makes sense, as makes it possible this may want to
clear the limit, only for the very next bit to reinstate it because it should
never have been cleared with the updated setting.
As I said, while applying the settings values, I would use a flag for each
of pageLimitMessages and the estimated max pages, to remember whether they have
changed....then at end, after application, check things when both are effective
together.
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/paging/impl/PagingStoreImpl.java:
##########
@@ -289,9 +290,23 @@ public void applySetting(final AddressSettings
addressSettings) {
this.pageLimitBytes = null;
}
+ this.pageLimitMessages = addressSettings.getPageLimitMessages();
+ if (firstTime || !Objects.equals(this.pageLimitMessages,
originalLimitMessages)) {
+ if (this.cursorProvider != null) {
+ this.cursorProvider.checkClearPageLimit();
+ }
+ }
+
if (pageLimitBytes != null && pageSize > 0) {
+ Long originalEstimatedMaxPages = this.estimatedMaxPages;
estimatedMaxPages = pageLimitBytes / pageSize;
logger.debug("Address {} should not allow more than {} pages",
storeName, estimatedMaxPages);
+ if (firstTime || !Objects.equals(estimatedMaxPages,
originalEstimatedMaxPages)) {
+ checkNumberOfPages();
+ if (cursorProvider != null) {
+ cursorProvider.checkClearPageLimit();
+ }
+ }
Review Comment:
Similarly, since start() checks at least the page count, and any use of the
address checks the message limits, the suggestion was that it not do this for
the constructor., but only later if the settings have changed. This instead
always does it for the constructor.
--
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
