clebertsuconic commented on code in PR #4752:
URL: https://github.com/apache/activemq-artemis/pull/4752#discussion_r1462332213
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/ScheduledDeliveryHandlerImpl.java:
##########
@@ -188,6 +194,34 @@ private void scheduleDelivery(final long deliveryTime) {
}
}
+ protected void notifyScheduledReferencesUpdated() {
+ oldestMessage = null;
+ }
+
+ @Override
+ public MessageReference peekFirstScheduledMessage() {
+ synchronized (scheduledReferences) {
+ if (scheduledReferences.isEmpty()) {
+ return null;
+ }
+ if (oldestMessage != null) {
+ return oldestMessage;
+ }
+ MessageReference result = null;
+ long oldestTimestamp = Long.MAX_VALUE;
+ for (RefScheduled scheduledReference : scheduledReferences) {
+ MessageReference ref = scheduledReference.getRef();
+ long refTimestamp = ref.getMessage().getTimestamp();
+ if (refTimestamp < oldestTimestamp) {
+ oldestTimestamp = refTimestamp;
+ result = ref;
+ }
+ }
+ oldestMessage = result;
+ return result;
+ }
+ }
Review Comment:
```suggestion
@Override
public MessageReference peekFirstScheduledMessage() {
synchronized (scheduledReferences) {
if (scheduledReferences.isEmpty()) {
return null;
}
RefScheduled refScheduled = scheduledReferences.first();
return refScheduled != null ? refScheduled.getRef() : null;
}
}
```
Why not just use scheduledReferences.first()?
I have been dealing with memory leaks in the past, especially the ones that
I tested with the leak-tests I wrote, and this would actually eventually leak
one message. Besides I think it's more efficient and less error prone to just
call getfirst().
in My suggestion I'm still checking for isEmpty(), which I'm not sure it's
needed.. something to verify.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
