GEODE-10010: Sample Redis ops per second during operations in test #7358
Conversation
DonalEvans
force-pushed
the
GEODE-10010-opsPerSecondStat
branch
2 times, most recently
from
b065cf7 to
bed9338
Compare
- Rather than a value that updates once per second, instantaneous operations per second and instantaneous kilobytes read per second now return a rolling average of those stats updated 16 times per second - Rename AbstractRedisInfoStatsIntegrationTest to match child classes - Rather than measuring instantaneous per second stats after operations have finished, sample them while operations are ongoing and calculate the expected value based on the number of operations performed in the one second prior to sampling. Authored-by: Donal Evans <[email protected]>
DonalEvans
force-pushed
the
GEODE-10010-opsPerSecondStat
branch
from
bed9338 to
35e4494
Compare
DonalEvans
requested review from
jdeppe-pivotal,
nonbinaryprogrammer,
ringles,
upthewaterspout and
dschneider-pivotal
as code owners
geode-for-redis/src/main/java/org/apache/geode/redis/internal/statistics/RedisStats.java
Outdated
Show resolved
Hide resolved
geode-for-redis/src/main/java/org/apache/geode/redis/internal/statistics/RedisStats.java
Outdated
Show resolved
Hide resolved
- Rename rollingAverageSamplesPerSecond to use all-caps - Extract rolling average calculations to static nested class Authored-by: Donal Evans <[email protected]>
Authored-by: Donal Evans <[email protected]>
| Arrays.stream(networkBytesReadOverLastNSamples).sum() / 1024.0; | ||
| totalNetworkBytesReadLastTick = totalNetworkBytesRead; | ||
| } | ||
| private static class RollingUpgradeStat { |
There was a problem hiding this comment.
should this be named "RollingAverageStat"?
There was a problem hiding this comment.
Yes, I was apparently on auto-pilot when I typed that, thanks for catching it
| private int rollingAverageTick = 0; | ||
| private final RollingUpgradeStat networkKiloBytesReadRollingAverageStat = |
There was a problem hiding this comment.
should this be named "Bytes" instead of "KiloBytes" to make clear that the units it computes is bytes and the caller needs to convert then to kilobytes by dividing by 1024?
Authored-by: Donal Evans <[email protected]>
- Reduce poll interval to 10ms in awaits - Retrieve stats before calculating expected value - Make variables final Authored-by: Donal Evans <[email protected]>
| .calculate(getTotalNetworkBytesRead(), rollingAverageTick) / 1024.0; | ||
| opsPerformedOverLastSecond = | ||
| opsPerformedRollingAverageStat.calculate(getCommandsProcessed(), rollingAverageTick); | ||
| rollingAverageTick++; |
There was a problem hiding this 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
There was a problem hiding this comment.
I like this idea. Since the tick count is inherently tied to the samples per second, which is also used when determining the delay for the ScheduledExecutorService, it's not possible to have different instances of RollingAverageStat with different number of samples to average over without also having a different instance of ScheduledExecutorService to execute them, but it still cleans things up to move the tick logic into RollingAverageStat.
To make the RollingAverageStat instances more self-contained, I also moved the call to get the current stat value inside the class, so all you need to do is create an instance, then call a zero-argument calculate() method to get the latest average.
- Take a max tick count and a getCurrentValue callable into the constructor - Move tick count management into the calculate() method - Move getting current value into the calculate() method Authored-by: Donal Evans <[email protected]>
Code changes: - Rather than a value that updates once per second, instantaneous operations per second and instantaneous kilobytes read per second now return a rolling average of those stats updated 16 times per second - Introduce RollingAverageStat nested class in RedisStats Test changes: - Rename AbstractRedisInfoStatsIntegrationTest to match child classes - Rather than measuring instantaneous per second stats after operations have finished, sample them while operations are ongoing and calculate the expected value based on the number of operations performed in the one second prior to sampling. - Change test tolerances from fixed value to 12.5% of expected value Authored-by: Donal Evans <[email protected]>
…he#7358)" (apache#7415) This reverts commit ea20f25.
operations per second and instantaneous kilobytes read per second now
return a rolling average of those stats updated 16 times per second
operations have finished, sample them while operations are ongoing and
calculate the expected value based on the number of operations
performed in the one second prior to sampling.
Authored-by: Donal Evans [email protected]
For all changes:
Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?
Has your PR been rebased against the latest commit within the target branch (typically
develop)?Is your initial contribution a single, squashed commit?
Does
gradlew buildrun cleanly?Have you written or updated unit tests to verify your changes?
If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?