cshannon commented on code in PR #1329:
URL: https://github.com/apache/activemq/pull/1329#discussion_r1887704821
##########
activemq-broker/src/main/java/org/apache/activemq/broker/jmx/DestinationView.java:
##########
@@ -620,4 +621,67 @@ public long getNetworkDequeues() {
return
destination.getDestinationStatistics().getNetworkDequeues().getCount();
}
+ @Override
+ public boolean isAdvancedMessageStatisticsEnabled() {
+ return destination.isAdvancedMessageStatisticsEnabled();
+ }
+
+ @Override
+ public void setAdvancedMessageStatisticsEnabled(boolean
advancedMessageStatisticsEnabled) {
+
destination.setAdvancedMessageStatisticsEnabled(advancedMessageStatisticsEnabled);
+ }
+
+ @Override
+ public long getEnqueuedMessageBrokerInTime() {
+ MessageFlowStats tmpMessageFlowStats =
destination.getDestinationStatistics().getMessageFlowStats();
+ return (tmpMessageFlowStats != null ?
tmpMessageFlowStats.getEnqueuedMessageBrokerInTime().getValue() : 0l);
+ }
+
+ @Override
+ public String getEnqueuedMessageClientId() {
+ MessageFlowStats tmpMessageFlowStats =
destination.getDestinationStatistics().getMessageFlowStats();
+ return (tmpMessageFlowStats != null ?
tmpMessageFlowStats.getEnqueuedMessageClientID().getValue() : null);
+ }
+
+ @Override
+ public String getEnqueuedMessageId() {
+ MessageFlowStats tmpMessageFlowStats =
destination.getDestinationStatistics().getMessageFlowStats();
+ return (tmpMessageFlowStats != null ?
tmpMessageFlowStats.getEnqueuedMessageID().getValue() : null);
+ }
+
+ @Override
+ public long getEnqueuedMessageTimestamp() {
+ MessageFlowStats tmpMessageFlowStats =
destination.getDestinationStatistics().getMessageFlowStats();
+ return (tmpMessageFlowStats != null ?
tmpMessageFlowStats.getEnqueuedMessageTimestamp().getValue() : 0l);
+ }
+
+ @Override
+ public long getDequeuedMessageBrokerInTime() {
+ MessageFlowStats tmpMessageFlowStats =
destination.getDestinationStatistics().getMessageFlowStats();
+ return (tmpMessageFlowStats != null ?
tmpMessageFlowStats.getDequeuedMessageBrokerInTime().getValue() : 0l);
+ }
+
+ @Override
+ public long getDequeuedMessageBrokerOutTime() {
+ MessageFlowStats tmpMessageFlowStats =
destination.getDestinationStatistics().getMessageFlowStats();
+ return (tmpMessageFlowStats != null ?
tmpMessageFlowStats.getDequeuedMessageBrokerOutTime().getValue() : 0l);
+ }
+
+ @Override
+ public String getDequeuedMessageClientId() {
+ MessageFlowStats tmpMessageFlowStats =
destination.getDestinationStatistics().getMessageFlowStats();
+ return (tmpMessageFlowStats != null ?
tmpMessageFlowStats.getDequeuedMessageClientID().getValue() : null);
+ }
+
+ @Override
+ public String getDequeuedMessageId() {
+ MessageFlowStats tmpMessageFlowStats =
destination.getDestinationStatistics().getMessageFlowStats();
+ return (tmpMessageFlowStats != null ?
tmpMessageFlowStats.getDequeuedMessageID().getValue() : null);
+ }
+
+ @Override
+ public long getDequeuedMessageTimestamp() {
Review Comment:
I was thinking all of these getters could be a bit cleaner and less
copy/paste with using lambdas. IE.
```java
@Override
public long getDequeuedMessageTimestamp() {
return getMessageFlowStat(MessageFlowStats::getDequeuedMessageTimestamp,
0L);
}
private <T> T getMessageFlowStat(Function<MessageFlowStats,
UnsampledStatistic<T>> f, T defVal) {
final var stats =
destination.getDestinationStatistics().getMessageFlowStats();
return stats != null ? f.apply(stats).getValue() : defVal;
}
```
##########
activemq-broker/src/main/java/org/apache/activemq/broker/region/DestinationStatistics.java:
##########
@@ -186,6 +177,11 @@ public void reset() {
maxUncommittedExceededCount.reset();
networkEnqueues.reset();
networkDequeues.reset();
+
+ MessageFlowStatsImpl tmpMessageFlowStats = messageFlowStats;
Review Comment:
Nit: I usually like to make all of these final just to prevent any future
mistakes
##########
activemq-broker/src/main/java/org/apache/activemq/broker/region/Queue.java:
##########
@@ -1971,10 +1978,17 @@ private final boolean tryCursorAdd(final Message msg)
throws Exception {
final void messageSent(final ConnectionContext context, final Message msg)
throws Exception {
pendingSends.decrementAndGet();
+
destinationStatistics.getEnqueues().increment();
destinationStatistics.getMessages().increment();
destinationStatistics.getMessageSize().addSize(msg.getSize());
+ MessageFlowStats tmpMessageFlowStats =
destinationStatistics.getMessageFlowStats();
+
+ if(isAdvancedMessageStatisticsEnabled() && tmpMessageFlowStats !=
null) {
Review Comment:
This is something where both of these should ALWAYS be in sync. you should
never have the stats enabled but the object is null.
Maybe the solution is you drop the flag entirely. Maybe stats being enabled
is really just a "is this object not null" and disabling is making the stats
null so we don't have to check 2 objects.
##########
activemq-broker/src/main/java/org/apache/activemq/broker/region/DestinationStatistics.java:
##########
@@ -252,7 +255,22 @@ public void setParent(DestinationStatistics parent) {
maxUncommittedExceededCount.setParent(null);
networkEnqueues.setParent(null);
networkDequeues.setParent(null);
+ // [AMQ-9437] Advanced Message Statistics do not parent.
}
}
+ public synchronized void setAdvancedMessageStatisticsEnabled(boolean
advancedMessageStatisticsEnabled) {
Review Comment:
There is a lot of thread safety issues in this entire class. By making all
of this stuff protected someone could easily screw up the state outside a lock.
Furthermore you are synchronizing this method but not other places like reset()
or anywhere else.
You should not be allowing the ability to set messageFlowStats or to set
advancedMessageStatisticsEnabled() outside this method as you could get them
out of sync and right now it's easy to do because they are not private
I need to think about it more but I really don't think the current approach
is great, especially if you are making member variables protected and not
private.
--
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
