You are viewing a plain text version of this content. The canonical link for it is here.
Posted to gitbox@activemq.apache.org by "jbertram (via GitHub)" <gi...@apache.org> on 2025/02/13 14:57:02 UTC
[PR] ARTEMIS-5302 use QueueConfiguration more [activemq-artemis]
jbertram opened a new pull request, #5504:
URL: https://github.com/apache/activemq-artemis/pull/5504
There are several places across the code-base that repeat all the configuration parameters of a queue. This commit simplifies the code and increases readability by using o.a.a.a.a.c.QueueConfiguration as often as possible.
--
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-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] ARTEMIS-5302 use QueueConfiguration more [activemq-artemis]
Posted by "jbertram (via GitHub)" <gi...@apache.org>.
jbertram commented on PR #5504:
URL: https://github.com/apache/activemq-artemis/pull/5504#issuecomment-2656865958
My only concern with this PR is that some fields in `QueueImpl` which were `volatile` have been replaced with ones from `QueueConfiguration` which are not. That said, the full test-suite is green on this so it's not clear that actually makes a difference. I would love feedback on this point specifically.
--
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-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] ARTEMIS-5302 use QueueConfiguration more [activemq-artemis]
Posted by "clebertsuconic (via GitHub)" <gi...@apache.org>.
clebertsuconic merged PR #5504:
URL: https://github.com/apache/activemq-artemis/pull/5504
--
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-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] ARTEMIS-5302 use QueueConfiguration more [activemq-artemis]
Posted by "clebertsuconic (via GitHub)" <gi...@apache.org>.
clebertsuconic commented on PR #5504:
URL: https://github.com/apache/activemq-artemis/pull/5504#issuecomment-2657145560
I looked now and it looks good to me.
I don't think the volatile was needed. The place where it's being used (QueueImpl.addConsumer) is under a synchronized block.
One thing that bothered me was beyond the scope of this PR.. as Queueimpl.addConsumer is called within ServerConsumerImpl... but that goes beyond the scope of this PR.
I don't have any issues with this PR if tests are good
--
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-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] ARTEMIS-5302 use QueueConfiguration more [activemq-artemis]
Posted by "jbertram (via GitHub)" <gi...@apache.org>.
jbertram commented on PR #5504:
URL: https://github.com/apache/activemq-artemis/pull/5504#issuecomment-2659805690
@gemmellr, I opened #5507 to deal with the issues you commented on here.
--
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-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] ARTEMIS-5302 use QueueConfiguration more [activemq-artemis]
Posted by "gemmellr (via GitHub)" <gi...@apache.org>.
gemmellr commented on code in PR #5504:
URL: https://github.com/apache/activemq-artemis/pull/5504#discussion_r1956106431
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/QueueImpl.java:
##########
@@ -4401,35 +4307,7 @@ public static MessageGroups<Consumer> groupMap(int groupBuckets) {
@Override
public QueueConfiguration getQueueConfiguration() {
- return QueueConfiguration.of(name)
- .setAddress(address)
- .setId(id)
- .setRoutingType(routingType)
- .setFilterString(filter == null ? null : filter.getFilterString())
- .setDurable(isDurable())
- .setUser(user)
- .setMaxConsumers(maxConsumers)
- .setExclusive(exclusive)
- .setGroupRebalance(groupRebalance)
- .setGroupBuckets(groupBuckets)
- .setGroupFirstKey(groupFirstKey)
- .setLastValue(false)
- .setLastValueKey((String) null)
- .setNonDestructive(nonDestructive)
- .setPurgeOnNoConsumers(purgeOnNoConsumers)
- .setConsumersBeforeDispatch(consumersBeforeDispatch)
- .setDelayBeforeDispatch(delayBeforeDispatch)
- .setAutoDelete(autoDelete)
- .setAutoDeleteDelay(autoDeleteDelay)
- .setAutoDeleteMessageCount(autoDeleteMessageCount)
- .setRingSize(ringSize)
- .setConfigurationManaged(configurationManaged)
- .setTemporary(temporary)
- .setInternal(internalQueue)
- .setTransient(refCountForConsumers instanceof TransientQueueManagerImpl)
- .setAutoCreated(autoCreated)
- .setEnabled(enabled)
- .setGroupRebalancePauseDispatch(groupRebalancePauseDispatch);
+ return queueConfiguration;
Review Comment:
Because this method isnt synchronized, switching to this actually is applying a behaviour change around all of those volatile fields that were removed. It will depend on how they and this return value are used specifically what that really means in practice.
##########
artemis-commons/src/test/java/org/apache/activemq/artemis/utils/SimpleStringTest.java:
##########
@@ -38,4 +40,23 @@ public void testOutOfBoundsThrownOnMalformedString() {
}
assertInstanceOf(IndexOutOfBoundsException.class, e);
}
+
+ @Test
+ public void testBlank() {
+ for (int i = 0; i <= 10; i++) {
+ assertTrue(SimpleString.of(" ".repeat(i)).isBlank());
+ }
+ for (int i = 0; i <= 10; i++) {
+ assertTrue(SimpleString.of("\t".repeat(i)).isBlank());
+ }
+ for (int i = 0; i <= 10; i++) {
+ assertTrue(SimpleString.of("\n".repeat(i)).isBlank());
+ }
+ for (int i = 0; i <= 10; i++) {
+ assertTrue(SimpleString.of("\r".repeat(i)).isBlank());
+ }
+ for (int i = 1; i <= 10; i++) {
+ assertFalse(SimpleString.of("x".repeat(i)).isBlank());
+ }
+ }
Review Comment:
This misses testing various interesting corner cases such as the actual-empty string which is also blank, and having whitespace at start, end, or both but not the whole string.
##########
artemis-commons/src/main/java/org/apache/activemq/artemis/api/core/QueueConfiguration.java:
##########
@@ -140,11 +140,6 @@ public static QueueConfiguration of(final QueueConfiguration queueConfiguration)
return new QueueConfiguration(queueConfiguration);
}
- /**
- * @deprecated
- * Use {@link #of(String)} instead.
- */
- @Deprecated(forRemoval = true)
Review Comment:
Rather than un-deprecating this, which only encourages its use (either brand new use, or continued use that missed the deprecation during the intermediate versions) instead of all the replacements that have been added, I think you should have left it deprecated and either suppressed the warning for the PersistentQueueBindingEncoding usage, or adjusted PersistentQueueBindingEncoding/its-uses such that those didnt need to call this.
##########
artemis-commons/src/main/java/org/apache/activemq/artemis/api/core/QueueConfiguration.java:
##########
@@ -416,12 +411,16 @@ public SimpleString getFilterString() {
}
public QueueConfiguration setFilterString(SimpleString filterString) {
- this.filterString = filterString;
+ if (filterString != null && !filterString.isEmpty() && !filterString.isBlank()) {
+ this.filterString = filterString;
+ } else if (filterString == null) {
+ this.filterString = filterString;
+ }
Review Comment:
This seems questionable, the setter will silently no-op if passed an empty or blank value, even if an existing value was previously set. So if called with empty or blank when nothing was set, the getter will then return null...and if called after a previous 'populating' set, the getter will then return the old value. In both cases, not matching the value most recently set.
Seems more like it should either allow the set (i.e continue expecting usages to handle the empty/blank return as it originally did), or else always treat empty/blank the same as setting null and clear any existing value.
--
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-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] ARTEMIS-5302 use QueueConfiguration more [activemq-artemis]
Posted by "jbertram (via GitHub)" <gi...@apache.org>.
jbertram commented on code in PR #5504:
URL: https://github.com/apache/activemq-artemis/pull/5504#discussion_r1956376868
##########
artemis-commons/src/test/java/org/apache/activemq/artemis/utils/SimpleStringTest.java:
##########
@@ -38,4 +40,23 @@ public void testOutOfBoundsThrownOnMalformedString() {
}
assertInstanceOf(IndexOutOfBoundsException.class, e);
}
+
+ @Test
+ public void testBlank() {
+ for (int i = 0; i <= 10; i++) {
+ assertTrue(SimpleString.of(" ".repeat(i)).isBlank());
+ }
+ for (int i = 0; i <= 10; i++) {
+ assertTrue(SimpleString.of("\t".repeat(i)).isBlank());
+ }
+ for (int i = 0; i <= 10; i++) {
+ assertTrue(SimpleString.of("\n".repeat(i)).isBlank());
+ }
+ for (int i = 0; i <= 10; i++) {
+ assertTrue(SimpleString.of("\r".repeat(i)).isBlank());
+ }
+ for (int i = 1; i <= 10; i++) {
+ assertFalse(SimpleString.of("x".repeat(i)).isBlank());
+ }
+ }
Review Comment:
The empty case is already covered by any assertion that uses `repeat(0)`. I'll add the others.
--
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-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact