Revert "Emit a success factor from maintainers"

5 files changed, 33 insertions, 63 deletions

diff --git a/vespajlib/src/main/java/com/yahoo/concurrent/maintenance/JobMetrics.java b/vespajlib/src/main/java/com/yahoo/concurrent/maintenance/JobMetrics.java
index 73a5dc77743..fcc5b8e57a2 100644
--- a/vespajlib/src/main/java/com/yahoo/concurrent/maintenance/JobMetrics.java
+++ b/vespajlib/src/main/java/com/yahoo/concurrent/maintenance/JobMetrics.java
@@ -2,18 +2,25 @@
 package com.yahoo.concurrent.maintenance;
 
 import java.util.concurrent.ConcurrentHashMap;
+import java.util.function.BiConsumer;
 
 /**
  * Tracks and forwards maintenance job metrics.
  *
  * @author mpolden
  */
-public abstract class JobMetrics {
+public class JobMetrics {
+
+    private final BiConsumer<String, Long> metricConsumer;
 
     private final ConcurrentHashMap<String, Long> incompleteRuns = new ConcurrentHashMap<>();
 
-    /** Record starting of a run of a job */
-    public void starting(String job) {
+    public JobMetrics(BiConsumer<String, Long> metricConsumer) {
+        this.metricConsumer = metricConsumer;
+    }
+
+    /** Record a run for given job */
+    public void recordRunOf(String job) {
         incompleteRuns.merge(job, 1L, Long::sum);
     }
 
@@ -22,17 +29,12 @@ public abstract class JobMetrics {
         incompleteRuns.put(job, 0L);
     }
 
-    /**
-     * Records completion of a run of a job.
-     * This is guaranteed to always be called once whenever starting has been called.
-     */
-    public void completed(String job, double successFactor) {
+    /** Forward metrics for given job to metric consumer */
+    public void forward(String job) {
         Long incompleteRuns = this.incompleteRuns.get(job);
         if (incompleteRuns != null) {
-            recordCompletion(job, incompleteRuns, successFactor);
+            metricConsumer.accept(job, incompleteRuns);
         }
     }
 
-    protected abstract void recordCompletion(String job, Long incompleteRuns, double successFactor);
-
 }
diff --git a/vespajlib/src/main/java/com/yahoo/concurrent/maintenance/Maintainer.java b/vespajlib/src/main/java/com/yahoo/concurrent/maintenance/Maintainer.java
index 2a9e6dda6b6..734c46a2819 100644
--- a/vespajlib/src/main/java/com/yahoo/concurrent/maintenance/Maintainer.java
+++ b/vespajlib/src/main/java/com/yahoo/concurrent/maintenance/Maintainer.java
@@ -83,19 +83,8 @@ public abstract class Maintainer implements Runnable {
     @Override
     public final String toString() { return name(); }
 
-    /**
-     * Called once each time this maintenance job should run.
-     *
-     * @return the degree to which the run was successful - a number between 0 (no success), to 1 (complete success).
-     *         Note that this indicates whether something is wrong, so e.g if the call did nothing because it should do
-     *         nothing,  1.0 should be returned.
-     */
-    protected abstract double maintain();
-
-    /** Convenience methods to convert attempts and failures into a success factor */
-    protected final double asSuccessFactor(int attempts, int failures) {
-        return attempts == 0 ? 1.0 : 1 - (double)failures / attempts;
-    }
+    /** Called once each time this maintenance job should run. Returns whether the maintenance run was successful */
+    protected abstract boolean maintain();
 
     /** Returns the interval at which this job is set to run */
     protected Duration interval() { return interval; }
@@ -104,12 +93,9 @@ public abstract class Maintainer implements Runnable {
     public final void lockAndMaintain(boolean force) {
         if (!force && !jobControl.isActive(name())) return;
         log.log(Level.FINE, () -> "Running " + this.getClass().getSimpleName());
-        jobMetrics.starting(name());
-        double successFactor = 0;
+        jobMetrics.recordRunOf(name());
         try (var lock = jobControl.lockJob(name())) {
-            successFactor = maintain();
-            if (successFactor > 0.0)
-                jobMetrics.recordCompletionOf(name());
+            if (maintain()) jobMetrics.recordCompletionOf(name());
         } catch (UncheckedTimeoutException e) {
             if (ignoreCollision) {
                 jobMetrics.recordCompletionOf(name());
@@ -119,7 +105,7 @@ public abstract class Maintainer implements Runnable {
         } catch (Throwable e) {
             log.log(Level.WARNING, this + " failed. Will retry in " + interval, e);
         } finally {
-            jobMetrics.completed(name(), successFactor);
+            jobMetrics.forward(name());
         }
         log.log(Level.FINE, () -> "Finished " + this.getClass().getSimpleName());
     }
diff --git a/vespajlib/src/test/java/com/yahoo/concurrent/maintenance/JobControlTest.java b/vespajlib/src/test/java/com/yahoo/concurrent/maintenance/JobControlTest.java
index 01560c050ff..139a2901cd3 100644
--- a/vespajlib/src/test/java/com/yahoo/concurrent/maintenance/JobControlTest.java
+++ b/vespajlib/src/test/java/com/yahoo/concurrent/maintenance/JobControlTest.java
@@ -3,8 +3,6 @@ package com.yahoo.concurrent.maintenance;
 
 import org.junit.Test;
 
-import java.util.concurrent.atomic.AtomicLong;
-
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertTrue;
@@ -21,8 +19,9 @@ public class JobControlTest {
 
         String job1 = "Job1";
         String job2 = "Job2";
-        TestMaintainer maintainer1 = new TestMaintainer(job1, jobControl, new NoopJobMetrics());
-        TestMaintainer maintainer2 = new TestMaintainer(job2, jobControl, new NoopJobMetrics());
+        JobMetrics metrics = new JobMetrics((job, instant) -> {});
+        TestMaintainer maintainer1 = new TestMaintainer(job1, jobControl, metrics);
+        TestMaintainer maintainer2 = new TestMaintainer(job2, jobControl, metrics);
         assertEquals(2, jobControl.jobs().size());
         assertTrue(jobControl.jobs().contains(job1));
         assertTrue(jobControl.jobs().contains(job2));
@@ -63,7 +62,7 @@ public class JobControlTest {
     public void testJobControlMayDeactivateJobs() {
         JobControlStateMock state = new JobControlStateMock();
         JobControl jobControl = new JobControl(state);
-        TestMaintainer mockMaintainer = new TestMaintainer(null, jobControl, new NoopJobMetrics());
+        TestMaintainer mockMaintainer = new TestMaintainer(null, jobControl, new JobMetrics((job, instant) -> {}));
 
         assertTrue(jobControl.jobs().contains("TestMaintainer"));
 
@@ -81,11 +80,4 @@ public class JobControlTest {
         assertEquals(2, mockMaintainer.totalRuns());
     }
 
-    private static class NoopJobMetrics extends JobMetrics {
-
-        @Override
-        protected void recordCompletion(String job, Long incompleteRuns, double successFactor) { }
-
-    }
-
 }
diff --git a/vespajlib/src/test/java/com/yahoo/concurrent/maintenance/MaintainerTest.java b/vespajlib/src/test/java/com/yahoo/concurrent/maintenance/MaintainerTest.java
index d2db380f4a1..e881d4b3ff6 100644
--- a/vespajlib/src/test/java/com/yahoo/concurrent/maintenance/MaintainerTest.java
+++ b/vespajlib/src/test/java/com/yahoo/concurrent/maintenance/MaintainerTest.java
@@ -42,45 +42,35 @@ public class MaintainerTest {
 
     @Test
     public void success_metric() {
-        TestJobMetrics jobMetrics = new TestJobMetrics();
+        AtomicLong consecutiveFailures = new AtomicLong();
+        JobMetrics jobMetrics = new JobMetrics((job, count) -> consecutiveFailures.set(count));
         TestMaintainer maintainer = new TestMaintainer(null, jobControl, jobMetrics);
 
         // Maintainer fails twice in a row
         maintainer.successOnNextRun(false).run();
-        assertEquals(1, jobMetrics.consecutiveFailures.get());
+        assertEquals(1, consecutiveFailures.get());
         maintainer.successOnNextRun(false).run();
-        assertEquals(2, jobMetrics.consecutiveFailures.get());
+        assertEquals(2, consecutiveFailures.get());
 
         // Maintainer runs successfully
         maintainer.successOnNextRun(true).run();
-        assertEquals(0, jobMetrics.consecutiveFailures.get());
+        assertEquals(0, consecutiveFailures.get());
 
         // Maintainer runs successfully again
         maintainer.run();
-        assertEquals(0, jobMetrics.consecutiveFailures.get());
+        assertEquals(0, consecutiveFailures.get());
 
         // Maintainer throws
         maintainer.throwOnNextRun(new RuntimeException()).run();
-        assertEquals(1, jobMetrics.consecutiveFailures.get());
+        assertEquals(1, consecutiveFailures.get());
 
         // Maintainer recovers
         maintainer.throwOnNextRun(null).run();
-        assertEquals(0, jobMetrics.consecutiveFailures.get());
+        assertEquals(0, consecutiveFailures.get());
 
         // Lock exception is treated as a failure
         maintainer.throwOnNextRun(new UncheckedTimeoutException()).run();
-        assertEquals(1, jobMetrics.consecutiveFailures.get());
-    }
-
-    private static class TestJobMetrics extends JobMetrics {
-
-        AtomicLong consecutiveFailures = new AtomicLong();
-
-        @Override
-        protected void recordCompletion(String job, Long incompleteRuns, double successFactor) {
-            consecutiveFailures.set(incompleteRuns);
-        }
-
+        assertEquals(1, consecutiveFailures.get());
     }
 
 }
diff --git a/vespajlib/src/test/java/com/yahoo/concurrent/maintenance/TestMaintainer.java b/vespajlib/src/test/java/com/yahoo/concurrent/maintenance/TestMaintainer.java
index 7424b17cab2..44a00a37a83 100644
--- a/vespajlib/src/test/java/com/yahoo/concurrent/maintenance/TestMaintainer.java
+++ b/vespajlib/src/test/java/com/yahoo/concurrent/maintenance/TestMaintainer.java
@@ -33,10 +33,10 @@ class TestMaintainer extends Maintainer {
     }
 
     @Override
-    protected double maintain() {
+    protected boolean maintain() {
         if (exceptionToThrow != null) throw exceptionToThrow;
         totalRuns++;
-        return success ? 1.0 : 0.0;
+        return success;
     }
 
 }