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-unsubscr...@activemq.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscr...@activemq.apache.org
For additional commands, e-mail: gitbox-h...@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
