dschneider-pivotal commented on a change in pull request #7358:
URL: https://github.com/apache/geode/pull/7358#discussion_r814967559
##########
File path:
geode-for-redis/src/main/java/org/apache/geode/redis/internal/statistics/RedisStats.java
##########
@@ -194,43 +199,46 @@ public void changeUniquePatternSubscriptions(long delta) {
public void close() {
geodeRedisStats.close();
- stopPerSecondUpdater();
+ stopRollingAverageUpdater();
}
- private ScheduledExecutorService startPerSecondUpdater() {
- int INTERVAL = 1;
+ private ScheduledExecutorService startRollingAverageUpdater() {
+ long microsPerSecond = 1_000_000;
+ final long delayMicros = microsPerSecond /
ROLLING_AVERAGE_SAMPLES_PER_SECOND;
- ScheduledExecutorService perSecondExecutor =
- newSingleThreadScheduledExecutor("GemFireRedis-PerSecondUpdater-");
+ ScheduledExecutorService rollingAverageExecutor =
+
newSingleThreadScheduledExecutor("GemFireRedis-RollingAverageStatUpdater-");
- perSecondExecutor.scheduleWithFixedDelay(
- this::doPerSecondUpdates,
- INTERVAL,
- INTERVAL,
- SECONDS);
+
rollingAverageExecutor.scheduleWithFixedDelay(this::doRollingAverageUpdates,
delayMicros,
+ delayMicros, MICROSECONDS);
- return perSecondExecutor;
+ return rollingAverageExecutor;
}
- private void stopPerSecondUpdater() {
- perSecondExecutor.shutdownNow();
+ private void stopRollingAverageUpdater() {
+ rollingAverageExecutor.shutdownNow();
}
- private void doPerSecondUpdates() {
- updateNetworkKilobytesReadLastSecond();
- updateOpsPerformedOverLastSecond();
+ private void doRollingAverageUpdates() {
+ networkKiloBytesReadOverLastSecond = networkBytesReadRollingAverageStat
+ .calculate(getTotalNetworkBytesRead(), rollingAverageTick) / 1024.0;
+ opsPerformedOverLastSecond =
+ opsPerformedRollingAverageStat.calculate(getCommandsProcessed(),
rollingAverageTick);
+ rollingAverageTick++;
Review comment:
I'm not sure about this suggestion but I think it would be better if
"rollingAverageTick" was an instance var on RollingAverageStat. Then all this
tick manipulation code would happen inside RollingAverageStat.calculate. I know
this will use a little bit more memory and time but it does allow the
possibility of each RollingAverageStat to have its own number of samples it
will average. As it is now when you just look at the calculate method it is
unclear how the array index is managed and if we might have an out of bounds
index. If this code was moved to calculate it would be clearer
--
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: notifications-unsubscr...@geode.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
