Fix bug in batch fetch for last successful evaluation (#471)

The state filter should occur on the inner query, before the join. This affected alerts that have at least one successful run when the most recent is a failure.
ArpNetworking · Nov 20, 2020 · 50c97ef · 50c97ef
1 parent 2612c8b
commit 50c97ef
Show file tree

Hide file tree

Showing 2 changed files with 8 additions and 1 deletion.
diff --git a/app/com/arpnetworking/metrics/portal/alerts/impl/DatabaseAlertExecutionRepository.java b/app/com/arpnetworking/metrics/portal/alerts/impl/DatabaseAlertExecutionRepository.java
@@ -222,6 +222,7 @@ public ImmutableMap<UUID, JobExecution.Success<AlertEvaluationResult>> getLastSu
                 + " JOIN (SELECT alert_id, max(completed_at) completed_at"
                 + "         FROM portal.alert_executions"
                 + "         WHERE scheduled >= :scheduled"
+                + "           AND state = :state"
                 + "         GROUP BY alert_id) t2"
                 + " ON t1.alert_id = t2.alert_id AND t1.completed_at = t2.completed_at"
                 + " WHERE t1.organization_id = (SELECT id FROM portal.organizations WHERE uuid = :organization_uuid)";
@@ -242,8 +243,8 @@ public ImmutableMap<UUID, JobExecution.Success<AlertEvaluationResult>> getLastSu
                     .setRawSql(rawSql)
                     .setParameter("scheduled", maxLookback)
                     .setParameter("organization_uuid", organization.getId())
+                    .setParameter("state", AlertExecution.State.SUCCESS)
                     .where()
-                    .eq("state", AlertExecution.State.SUCCESS)
                     .gt("scheduled", maxLookback)
                     .in("alertId", jobIds)
                     .findList();

diff --git a/...a/com/arpnetworking/metrics/portal/integration/repositories/JobExecutionRepositoryIT.java b/...a/com/arpnetworking/metrics/portal/integration/repositories/JobExecutionRepositoryIT.java
@@ -352,11 +352,17 @@ public void testGetLastSuccessBatch() {
                 _repository.jobSucceeded(jobId, _organization, scheduled, result);
             }
         }
+        // Create a failed execution in the "future" for one job id
+        final Instant futureRun = truncatedNow.plus(Duration.ofDays(1));
+        _repository.jobStarted(existingJobIds.get(0), _organization, futureRun);
+        _repository.jobFailed(existingJobIds.get(0), _organization, futureRun, new Throwable("an error"));
+
         // Create an additional job that we don't care about.
         final UUID extraJobId = UUID.randomUUID();
         ensureJobExists(_organization, extraJobId);
         _repository.jobStarted(extraJobId, _organization, truncatedNow);
         _repository.jobSucceeded(extraJobId, _organization, truncatedNow, newResult());
+
         // Create an additional job with a failure.
         final UUID failedJobId = UUID.randomUUID();
         ensureJobExists(_organization, failedJobId);