ARTEMIS-5302 use QueueConfiguration more #5504
Conversation
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.
|
My only concern with this PR is that some fields in |
|
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 |
| /** | ||
| * @deprecated | ||
| * Use {@link #of(String)} instead. | ||
| */ | ||
| @Deprecated(forRemoval = true) |
There was a problem hiding this 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.
| if (filterString != null && !filterString.isEmpty() && !filterString.isBlank()) { | ||
| this.filterString = filterString; | ||
| } else if (filterString == null) { | ||
| this.filterString = filterString; | ||
| } |
There was a problem hiding this 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.
| 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()); | ||
| } | ||
| } |
There was a problem hiding this 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.
There was a problem hiding this comment.
The empty case is already covered by any assertion that uses repeat(0). I'll add the others.
| .setAutoCreated(autoCreated) | ||
| .setEnabled(enabled) | ||
| .setGroupRebalancePauseDispatch(groupRebalancePauseDispatch); | ||
| return queueConfiguration; |
There was a problem hiding this 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.
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.