-
Notifications
You must be signed in to change notification settings - Fork 916
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ARTEMIS-4809 Allow configuring initial queue buffer size #4966
Conversation
Thanks for the PR! I think this would be better as an address-setting so you could easily configure large swaths of queues. Right now there's no way to configure this via Lastly, there's no tests or documentation for this new setting. Tests are mandatory to ensure the functionality works properly and to mitigate future regressions. |
Thanks @jbertram! I have updated my PR to make this configurable on a per-address basis and via XML as well. I have also added tests and documentation for the functionality. Please let me know if there is a different approach I should be taking here. |
...mis-server/src/main/java/org/apache/activemq/artemis/core/settings/impl/AddressSettings.java
Outdated
Show resolved
Hide resolved
artemis-server/src/main/resources/schema/artemis-configuration.xsd
Outdated
Show resolved
Hide resolved
Checkstyle is fixed now as well. |
For what it's worth you can run checkstyle locally using:
See the hacking guide for more details. |
@jbertram thank you—it's strange since I did run |
Hi @jbertram, any feedback on this? |
The last failure you had on 1 of the 3 'check' jobs, was actually from ErrorProne rather than Checkstyle, due to missing an @ Override. There is a different ErrorProne profile used on JDK 11+ builds and JDK16+ (i.e the JDK 17 and 21 CI runs), and that latter profile also does not check the @ Override condition, because a Java 15 class signature change meant that one (or more, I forget) of the classes in the codebase actually violates that on Java 15+ but not on earlier versions (aside: I deliberately left 15 hanging when adapting the other profile for 16/17+, since it was out of support anyway). This came up because in the past, the JDK > 11 runs were all using their own newer compile signatures, because we couldnt set the maven.compiler.release flag at the time. That is actually recently in place, so theyll all now be using the JDK11 method signatures now, hopefully meaning that issue shouldnt be present any more and I can adjust it to error on missing @ Override again in the JDK16+ profile too for consistency. |
...mis-server/src/main/java/org/apache/activemq/artemis/core/settings/impl/AddressSettings.java
Outdated
Show resolved
Hide resolved
artemis-server/src/main/java/org/apache/activemq/artemis/core/server/ActiveMQMessageBundle.java
Outdated
Show resolved
Hide resolved
ErrorProne check on JDK16+ adjusted in 6584220 |
artemis-commons/src/main/java/org/apache/activemq/artemis/utils/RandomUtil.java
Outdated
Show resolved
Hide resolved
if ((value.longValue() & (value.longValue() - 1)) == 0 && value.longValue() > 0) { | ||
return value; | ||
} else { | ||
throw ActiveMQMessageBundle.BUNDLE.powerOfTwo(name, value); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if the validator is being called POSITIVE_POWER_OF_TWO would it make sense for the exception bundle method to be specific as well?
artemis-server/src/test/java/org/apache/activemq/artemis/core/config/impl/ValidatorsTest.java
Show resolved
Hide resolved
Spotted a few small niggles, nothing serious. Would like Justin or someone else more familiar to also review. |
Generally speaking I think this looks good. However, there's a couple of things I would revisit:
|
In some setups, there could be a few hundred thousand queues that are created due to many consumers that are connecting. However, most of these are empty and stay empty for the entire day since there aren't necessarily messages to be sent. The 8K intermediateMessageReferences instantiates an 64KB buffer (Object[]). This means we have large allocation and live heap that ultimately remains empty for almost the entire day. In this commit, we introduce initial-queue-buffer-size, which defaults to the current value of 8192. It can be set programmatically via QueueConfiguration#setInitialQueueBufferSize(int). Note that this must be a positive power of 2.
@jbertram I agree, that makes sense. I have revised to |
LGTM |
@joshb1050, thanks for the contribution! |
In some setups, there could be a few hundred thousand queues that are created due to many consumers that are connecting. However, most of these are empty and stay empty for the entire day since there aren't necessarily messages to be sent. The 8K
intermediateMessageReferences
instantiates an64KB
buffer (Object[]
). This means we have large allocation and live heap that ultimately remains empty for almost the entire day.In this commit, we introduce
initial-queue-buffer-size
, which defaults to the current value of8192
. It can be set programmatically viaQueueConfiguration#setInitialQueueBufferSize(int)
.Note that this must be a power of 2.