Copilot commented on code in PR #13659:
URL: https://github.com/apache/cloudstack/pull/13659#discussion_r3637961554
##########
server/src/main/java/com/cloud/server/StatsCollector.java:
##########
@@ -1286,13 +1286,21 @@ protected Point createInfluxDbPoint(Object
metricsObject) {
*/
class VmStatsCleaner extends ManagedContextRunnable{
protected void runInContext() {
Review Comment:
Add `@Override` for runInContext() to match the rest of the
ManagedContextRunnable inner classes in this file (e.g.,
VmDiskStatsTask/VmDiskStatsUpdaterTask). This prevents accidental signature
drift from silently creating a new method and losing the scheduled-task guard.
This issue also appears on line 1297 of the same file.
##########
server/src/test/java/com/cloud/server/StatsCollectorTest.java:
##########
@@ -335,6 +335,49 @@ public void cleanUpVirtualMachineStatsTestIsEnabled() {
Mockito.verify(vmStatsDaoMock).removeAllByTimestampLessThan(Mockito.any(),
Mockito.anyLong());
}
+ private void setVmDiskStatsMaxRetentionTimeValue(String value) {
+ StatsCollector.vmDiskStatsMaxRetentionTime = new
ConfigKey<Integer>("Advanced", Integer.class,
"vm.disk.stats.max.retention.time", value,
+ "The maximum time (in minutes) for keeping Volume stats
records in the database. The Volume stats cleanup process will be disabled if
this is set to 0 or less than 0.", true);
+ }
+
+ @Test
+ public void cleanUpVolumeStatsTestIsDisabled() {
+ setVmDiskStatsMaxRetentionTimeValue("0");
+
+ statsCollector.cleanUpVolumeStats();
+
+ Mockito.verify(volumeStatsDao,
Mockito.never()).removeAllByTimestampLessThan(Mockito.any(), Mockito.anyLong());
+ }
+
+ @Test
+ public void cleanUpVolumeStatsTestIsEnabled() {
+ setVmDiskStatsMaxRetentionTimeValue("1");
+
+ statsCollector.cleanUpVolumeStats();
+
+
Mockito.verify(volumeStatsDao).removeAllByTimestampLessThan(Mockito.any(),
Mockito.anyLong());
+ }
+
+ @Test
+ public void
vmStatsCleanerTestCatchesCloudRuntimeExceptionAndKeepsRunning() {
Review Comment:
This test name says it catches CloudRuntimeException and "keeps running",
but the production code now catches any RuntimeException and this unit test
only verifies that the exception does not propagate from a single run().
Consider renaming to reflect what is actually being asserted.
This issue also appears on line 371 of the same file.
--
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]
