kenliao94 commented on code in PR #1285:
URL: https://github.com/apache/activemq/pull/1285#discussion_r1728620006
##########
activemq-client/src/main/java/org/apache/activemq/management/StatisticImpl.java:
##########
@@ -80,6 +80,9 @@ public synchronized long getLastSampleTime() {
return this.lastSampleTime;
}
+ public synchronized long getLastSampleTimeOrStartTime(){
+ return lastSampleTime == 0 ? startTime : lastSampleTime;
Review Comment:
An alternative approach to setting the lastSampleTime to 0, we can keep the
`this.lastSampleTime = this.startTime;` as originally implemented. But then we
add an additional field in StatisticImpl class like "hasUpdated" to indicate if
the lastSampleTime has been updated. In that way, it's up to the caller to
decide what to make use of this "hasUpdated" field. I.E write 0, or not set at
all ... etc.
The advantage of avoiding giving special meaning to 0 is:
1. Not explicit in semantic and make the code a bit harder to follow (why 0?)
2. What if 0 means some other things in downstream code
3. More flexible and clear on the caller, it calls the object to see if the
lastSampleTime has been updated since restart, rather than just set the value
to whatever `stats.getEnqueues().getLastSampleTime()` returns
--
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
