Repository navigation
JAMES-4192 Merge Artemis delayed queue improvements with latest master - #3230
Open
prosgarz35 wants to merge 2 commits into
Open
prosgarz35 wants to merge 2 commits into
prosgarz35 wants to merge 2 commits into
Conversation
Closed
Contributor
Author
|
artemis -> activemq renaming for better backward compability |
prosgarz35
marked this pull request as ready for review
October 6, 2026 02:31
chibenwa
reviewed
Oct 6, 2026
chibenwa
left a comment
chibenwa
left a comment
Contributor
There was a problem hiding this comment.
This PR reverts parts of master, weakens durability by default, and its delayed-queue design causes heavy journal churn.
The idea (a companion -delayed queue, and no selector on the main queue) is sound. The implementation has problems:
- Very heavy write churn. Every 200 ms the transfer task drains up to 1000 messages and re-sends every message that isn't due yet, full MIME body included. Each delayed mail is
rewritten to the journal 5 times per second. With 1000 delayed mails of 100 KB, that's about 500 MB/s of journal writes. The javadoc calls the interval "coarse", but 200 ms is not. - Memory. Up to 1000 full messages are held in memory on every pass.
- Thread safety. enQueueDelayed calls delayedSession.createObjectMessage() outside the synchronized (delayedProducer) block. A JMS Session isn't thread-safe, and enqueues are
concurrent. - send(copy, mode, prio, m.getJMSExpiration()) passes an absolute expiration timestamp where the API expects a relative time-to-live. It's harmless while expiration is 0, but wrong.
- remove() on the delayed queue uses a selector. That's the same paging limitation the PR sets out to avoid. The value is also concatenated without escaping ', % and _.
- removeFromDelayedQueue waits receive(2000), so every clear() or remove() takes at least 2 s even when the delayed queue is empty.
- One extra thread per queue for the transfer task.
- Rejected alternatives aren't evaluated. The PR dismisses Artemis native scheduled delivery because of browse()/getSize() limits, but doesn't consider a non-polling design (for
example, delayed queues bucketed by delivery time).
Comment on lines
+100
to
+104
| <groupId>org.apache.artemis</groupId> | ||
| <groupId>org.apache.activemq</groupId> | ||
| <artifactId>artemis-jakarta-client</artifactId> | ||
| </dependency> | ||
| <dependency> | ||
| <groupId>org.apache.artemis</groupId> | ||
| <groupId>org.apache.activemq</groupId> |
Comment on lines
-4606
to
-4616
| <profile> | ||
| <id>windows-git</id> | ||
| <activation> | ||
| <os> | ||
| <family>windows</family> | ||
| </os> | ||
| </activation> | ||
| <properties> | ||
| <git.useNativeGit>false</git.useNativeGit> | ||
| </properties> | ||
| </profile> |
Comment on lines
+34
to
+53
| @@ -47,10 +47,10 @@ public class ActiveMQConfiguration { | |||
| private static final boolean ASYNC_CONNECTION_EXECUTION_DEFAULT = true; | |||
|
|
|||
| private static final String CLIENT_BLOCK_ON_DURABLE_SEND = "artemis.client.block.on.durable.send"; | |||
| private static final boolean CLIENT_BLOCK_ON_DURABLE_SEND_DEFAULT = true; | |||
| private static final boolean CLIENT_BLOCK_ON_DURABLE_SEND_DEFAULT = false; | |||
|
|
|||
| private static final String CLIENT_BLOCK_ON_ACKNOWLEDGE = "artemis.client.block.on.acknowledge"; | |||
| private static final boolean CLIENT_BLOCK_ON_ACKNOWLEDGE_DEFAULT = true; | |||
| private static final boolean CLIENT_BLOCK_ON_ACKNOWLEDGE_DEFAULT = false; | |||
| attrValue = message.getObjectProperty(name); | ||
| } | ||
| } catch (Exception e) { | ||
| } catch (JMSException e) { |
Contributor
There was a problem hiding this comment.
Why ?
catch (Exception) → catch (JMSException) in mailAttribute narrows the catch. A runtime exception (deserialization, for example) will now propagate instead of being logged, which can break dequeueing
| // Dequeue should instantly receive the ready message without head-of-line blocking | ||
| reactor.core.publisher.Mono<org.apache.james.queue.api.MailQueue.MailQueueItem> itemMono = | ||
| reactor.core.publisher.Flux.from(mailQueue.deQueue()).next(); | ||
| org.apache.james.queue.api.MailQueue.MailQueueItem dequeuedItem = |
Contributor
Author
|
@chibenwa i fixed this :) |
Contributor
|
Hello @prosgarz35 The latest 1d87744 commit handles only fixes for inline comments but do not seem to address #3230 (review) Cosmetic issues are addressed; fine. We would now need core design issue to get more work before being able to merge. Would you be able to push this work further? Best regards, Benoit TELLIER |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.