From 0c55dc92a3bf889c67fac1ca855e6e33e1994904 Mon Sep 17 00:00:00 2001 From: Jon Bratseth Date: Mon, 9 Oct 2023 09:44:29 +0200 Subject: Update copyright --- .../com/yahoo/vespa/hosted/controller/deployment/DeploymentTester.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentTester.java') diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentTester.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentTester.java index ee6a21d071d..1cc549ec6ca 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentTester.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentTester.java @@ -1,4 +1,4 @@ -// Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. +// Copyright Vespa.ai. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. package com.yahoo.vespa.hosted.controller.deployment; import com.yahoo.config.provision.ApplicationId; -- cgit v1.2.3 From 0f76f2837215f3202dab2ea1c84c53bbf5bd0e7f Mon Sep 17 00:00:00 2001 From: jonmv Date: Wed, 11 Oct 2023 13:37:03 +0200 Subject: Non-functional changes --- .../container/handler/threadpool/ContainerThreadpoolImpl.java | 2 +- .../threadpool/WorkerCompletionTimingThreadPoolExecutor.java | 3 +-- .../main/java/com/yahoo/container/protect/ProcessTerminator.java | 4 ++-- .../vespa/hosted/controller/deployment/DeploymentTester.java | 3 +-- .../documentapi/messagebus/protocol/AdaptiveLoadBalancer.java | 2 +- .../yahoo/documentapi/messagebus/protocol/LocalServicePolicy.java | 2 +- messagebus/src/main/java/com/yahoo/messagebus/ErrorCode.java | 8 ++++---- .../main/java/com/yahoo/messagebus/routing/RoutingContext.java | 2 +- 8 files changed, 12 insertions(+), 14 deletions(-) (limited to 'controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentTester.java') diff --git a/container-core/src/main/java/com/yahoo/container/handler/threadpool/ContainerThreadpoolImpl.java b/container-core/src/main/java/com/yahoo/container/handler/threadpool/ContainerThreadpoolImpl.java index befbda28ac0..f92d218390f 100644 --- a/container-core/src/main/java/com/yahoo/container/handler/threadpool/ContainerThreadpoolImpl.java +++ b/container-core/src/main/java/com/yahoo/container/handler/threadpool/ContainerThreadpoolImpl.java @@ -54,7 +54,7 @@ public class ContainerThreadpoolImpl extends AbstractComponent implements AutoCl createQueue(queueSize), ThreadFactoryFactory.getThreadFactory(name), threadPoolMetric); - // Prestart needed, if not all threads will be created by the fist N tasks and hence they might also + // Pre-start needed, if not all threads will be created by the fist N tasks and hence they might also // get the dreaded thread locals initialized even if they will never run. // That counters what we want to achieve with the Q that will prefer thread locality. executor.prestartAllCoreThreads(); diff --git a/container-core/src/main/java/com/yahoo/container/handler/threadpool/WorkerCompletionTimingThreadPoolExecutor.java b/container-core/src/main/java/com/yahoo/container/handler/threadpool/WorkerCompletionTimingThreadPoolExecutor.java index e3c2c78abec..cee2cc54b5b 100644 --- a/container-core/src/main/java/com/yahoo/container/handler/threadpool/WorkerCompletionTimingThreadPoolExecutor.java +++ b/container-core/src/main/java/com/yahoo/container/handler/threadpool/WorkerCompletionTimingThreadPoolExecutor.java @@ -8,8 +8,7 @@ import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicLong; /** - * A thread pool executor which maintains the last time a worker completed - * package private for testing + * A thread pool executor which maintains the last time a worker completed. * * @author Steinar Knutsen * @author baldersheim diff --git a/container-core/src/main/java/com/yahoo/container/protect/ProcessTerminator.java b/container-core/src/main/java/com/yahoo/container/protect/ProcessTerminator.java index 315dc21ec38..3c44ba2eaa6 100644 --- a/container-core/src/main/java/com/yahoo/container/protect/ProcessTerminator.java +++ b/container-core/src/main/java/com/yahoo/container/protect/ProcessTerminator.java @@ -4,8 +4,8 @@ package com.yahoo.container.protect; import com.yahoo.protect.Process; /** - * An injectable terminator of the Java vm. - * Components that encounters conditions where the vm should be terminated should + * An injectable terminator of the Java VM. + * Components that encounter conditions where the VM should be terminated should * request an instance of this injected. That makes termination testable * as tests can create subclasses of this which register the termination request * rather than terminating. diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentTester.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentTester.java index 1cc549ec6ca..c16234b3948 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentTester.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentTester.java @@ -75,8 +75,7 @@ public class DeploymentTester { tester = controllerTester; jobs = tester.controller().jobController(); cloud = (MockTesterCloud) tester.controller().jobController().cloud(); - runner = new JobRunner(tester.controller(), maintenanceInterval, JobRunnerTest.inThreadExecutor(), - new InternalStepRunner(tester.controller())); + runner = new JobRunner(tester.controller(), maintenanceInterval, JobRunnerTest.inThreadExecutor(), new InternalStepRunner(tester.controller())); upgrader = new Upgrader(tester.controller(), maintenanceInterval); upgrader.setUpgradesPerMinute(1); // Anything that makes it at least one for any maintenance period is fine. readyJobsTrigger = new ReadyJobsTrigger(tester.controller(), maintenanceInterval); diff --git a/documentapi/src/main/java/com/yahoo/documentapi/messagebus/protocol/AdaptiveLoadBalancer.java b/documentapi/src/main/java/com/yahoo/documentapi/messagebus/protocol/AdaptiveLoadBalancer.java index 4a4cf0fd5c8..ae934857e2c 100644 --- a/documentapi/src/main/java/com/yahoo/documentapi/messagebus/protocol/AdaptiveLoadBalancer.java +++ b/documentapi/src/main/java/com/yahoo/documentapi/messagebus/protocol/AdaptiveLoadBalancer.java @@ -7,7 +7,7 @@ import java.util.List; import java.util.Random; /** - * Will pick 2 random candidates and select the one with least pending operations. + * Will pick 2 random candidates and select the one with the least pending operations. * * @author baldersheim */ diff --git a/documentapi/src/main/java/com/yahoo/documentapi/messagebus/protocol/LocalServicePolicy.java b/documentapi/src/main/java/com/yahoo/documentapi/messagebus/protocol/LocalServicePolicy.java index ddd04a3ca53..4f8227b35a0 100755 --- a/documentapi/src/main/java/com/yahoo/documentapi/messagebus/protocol/LocalServicePolicy.java +++ b/documentapi/src/main/java/com/yahoo/documentapi/messagebus/protocol/LocalServicePolicy.java @@ -23,7 +23,7 @@ public class LocalServicePolicy implements DocumentProtocolRoutingPolicy { private final Map cache = new HashMap<>(); /** - * Constructs a policy that will choose local services that match the slobrok pattern in which this policy occured. + * Constructs a policy that will choose local services that match the slobrok pattern in which this policy occurred. * If no local service can be found, this policy simply returns the asterisk to allow the network to choose any. * * @param param The address to use for this, if empty this will resolve to hostname. diff --git a/messagebus/src/main/java/com/yahoo/messagebus/ErrorCode.java b/messagebus/src/main/java/com/yahoo/messagebus/ErrorCode.java index 8fab523d4ae..88e3e1a89bc 100644 --- a/messagebus/src/main/java/com/yahoo/messagebus/ErrorCode.java +++ b/messagebus/src/main/java/com/yahoo/messagebus/ErrorCode.java @@ -20,7 +20,7 @@ public final class ErrorCode { /** No addresses found for the services of the message route. */ public static final int NO_ADDRESS_FOR_SERVICE = TRANSIENT_ERROR + 2; - /** A connection problem occured while sending. */ + /** A connection problem occurred while sending. */ public static final int CONNECTION_ERROR = TRANSIENT_ERROR + 3; /** The session specified for the message is unknown. */ @@ -50,10 +50,10 @@ public final class ErrorCode { /** No services found for the message route. */ public static final int NO_SERVICES_FOR_ROUTE = FATAL_ERROR + 3; - /** An error occured while encoding the message. */ + /** An error occurred while encoding the message. */ public static final int ENCODE_ERROR = FATAL_ERROR + 5; - /** A fatal network error occured while sending. */ + /** A fatal network error occurred while sending. */ public static final int NETWORK_ERROR = FATAL_ERROR + 6; /** The protocol specified for the message is unknown. */ @@ -77,7 +77,7 @@ public final class ErrorCode { /** Exception thrown by routing policy. */ public static final int POLICY_ERROR = FATAL_ERROR + 13; - /** An error occured while sequencing a message. */ + /** An error occurred while sequencing a message. */ public static final int SEQUENCE_ERROR = FATAL_ERROR + 14; /** An application specific non-recoverable error. */ diff --git a/messagebus/src/main/java/com/yahoo/messagebus/routing/RoutingContext.java b/messagebus/src/main/java/com/yahoo/messagebus/routing/RoutingContext.java index 227dd546ad8..18b5de34bb4 100755 --- a/messagebus/src/main/java/com/yahoo/messagebus/routing/RoutingContext.java +++ b/messagebus/src/main/java/com/yahoo/messagebus/routing/RoutingContext.java @@ -19,7 +19,7 @@ public class RoutingContext { private final RoutingNode node; private final int directive; - private final Set consumableErrors = new HashSet(); + private final Set consumableErrors = new HashSet<>(); private boolean selectOnRetry = true; private Object context = null; -- cgit v1.2.3 From 34d35c3467aebeda68c3d58ccfb683c0f4f0c506 Mon Sep 17 00:00:00 2001 From: Harald Musum Date: Wed, 11 Oct 2023 18:53:38 +0200 Subject: Revert "Jonmv/job runner thread metrics" --- .../threadpool/ContainerThreadpoolImpl.java | 2 +- .../WorkerCompletionTimingThreadPoolExecutor.java | 3 +- .../yahoo/container/protect/ProcessTerminator.java | 4 +- .../controller/deployment/InternalStepRunner.java | 2 - .../hosted/controller/maintenance/JobRunner.java | 58 ++------------------- .../controller/deployment/DeploymentTester.java | 3 +- .../controller/maintenance/JobRunnerTest.java | 59 ---------------------- .../messagebus/protocol/AdaptiveLoadBalancer.java | 2 +- .../messagebus/protocol/LocalServicePolicy.java | 2 +- .../main/java/com/yahoo/messagebus/ErrorCode.java | 8 +-- .../yahoo/messagebus/routing/RoutingContext.java | 2 +- .../java/ai/vespa/metrics/ControllerMetrics.java | 2 - .../vespa/metrics/set/InfrastructureMetricSet.java | 2 - 13 files changed, 19 insertions(+), 130 deletions(-) (limited to 'controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentTester.java') diff --git a/container-core/src/main/java/com/yahoo/container/handler/threadpool/ContainerThreadpoolImpl.java b/container-core/src/main/java/com/yahoo/container/handler/threadpool/ContainerThreadpoolImpl.java index f92d218390f..befbda28ac0 100644 --- a/container-core/src/main/java/com/yahoo/container/handler/threadpool/ContainerThreadpoolImpl.java +++ b/container-core/src/main/java/com/yahoo/container/handler/threadpool/ContainerThreadpoolImpl.java @@ -54,7 +54,7 @@ public class ContainerThreadpoolImpl extends AbstractComponent implements AutoCl createQueue(queueSize), ThreadFactoryFactory.getThreadFactory(name), threadPoolMetric); - // Pre-start needed, if not all threads will be created by the fist N tasks and hence they might also + // Prestart needed, if not all threads will be created by the fist N tasks and hence they might also // get the dreaded thread locals initialized even if they will never run. // That counters what we want to achieve with the Q that will prefer thread locality. executor.prestartAllCoreThreads(); diff --git a/container-core/src/main/java/com/yahoo/container/handler/threadpool/WorkerCompletionTimingThreadPoolExecutor.java b/container-core/src/main/java/com/yahoo/container/handler/threadpool/WorkerCompletionTimingThreadPoolExecutor.java index cee2cc54b5b..e3c2c78abec 100644 --- a/container-core/src/main/java/com/yahoo/container/handler/threadpool/WorkerCompletionTimingThreadPoolExecutor.java +++ b/container-core/src/main/java/com/yahoo/container/handler/threadpool/WorkerCompletionTimingThreadPoolExecutor.java @@ -8,7 +8,8 @@ import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicLong; /** - * A thread pool executor which maintains the last time a worker completed. + * A thread pool executor which maintains the last time a worker completed + * package private for testing * * @author Steinar Knutsen * @author baldersheim diff --git a/container-core/src/main/java/com/yahoo/container/protect/ProcessTerminator.java b/container-core/src/main/java/com/yahoo/container/protect/ProcessTerminator.java index 3c44ba2eaa6..315dc21ec38 100644 --- a/container-core/src/main/java/com/yahoo/container/protect/ProcessTerminator.java +++ b/container-core/src/main/java/com/yahoo/container/protect/ProcessTerminator.java @@ -4,8 +4,8 @@ package com.yahoo.container.protect; import com.yahoo.protect.Process; /** - * An injectable terminator of the Java VM. - * Components that encounter conditions where the VM should be terminated should + * An injectable terminator of the Java vm. + * Components that encounters conditions where the vm should be terminated should * request an instance of this injected. That makes termination testable * as tests can create subclasses of this which register the termination request * rather than terminating. diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/InternalStepRunner.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/InternalStepRunner.java index 1080b379c4d..662b4018a34 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/InternalStepRunner.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/InternalStepRunner.java @@ -727,8 +727,6 @@ public class InternalStepRunner implements StepRunner { DeploymentSpec spec = controller.applications().requireApplication(TenantAndApplicationId.from(id.application())).deploymentSpec(); boolean requireTests = spec.steps().stream().anyMatch(step -> step.concerns(id.type().environment())); - logger.log(WARNING, "No tests were actually run, but this test suite is explicitly declared in 'deployment.xml'. " + - "Either add tests, ensure they're correctly configured, or remove the test declaration."); return Optional.of(requireTests ? testFailure : noTests); } case SUCCESS: diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/JobRunner.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/JobRunner.java index 0f482b1a015..6f00ff39637 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/JobRunner.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/JobRunner.java @@ -1,9 +1,7 @@ // Copyright Vespa.ai. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. package com.yahoo.vespa.hosted.controller.maintenance; -import ai.vespa.metrics.ControllerMetrics; import com.yahoo.concurrent.DaemonThreadFactory; -import com.yahoo.jdisc.Metric; import com.yahoo.vespa.hosted.controller.Controller; import com.yahoo.vespa.hosted.controller.api.integration.deployment.RunId; import com.yahoo.vespa.hosted.controller.deployment.InternalStepRunner; @@ -13,14 +11,11 @@ import com.yahoo.vespa.hosted.controller.deployment.Step; import com.yahoo.vespa.hosted.controller.deployment.StepRunner; import java.time.Duration; -import java.util.Map; import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; -import java.util.concurrent.ScheduledExecutorService; import java.util.concurrent.TimeUnit; import java.util.concurrent.TimeoutException; import java.util.concurrent.atomic.AtomicBoolean; -import java.util.concurrent.atomic.AtomicInteger; import java.util.logging.Level; import java.util.logging.Logger; @@ -37,29 +32,22 @@ public class JobRunner extends ControllerMaintainer { private final JobController jobs; private final ExecutorService executors; private final StepRunner runner; - private final Metrics metrics; public JobRunner(Controller controller, Duration duration) { - this(controller, duration, Executors.newFixedThreadPool(32, new DaemonThreadFactory("job-runner-")), - new InternalStepRunner(controller)); + this(controller, duration, Executors.newFixedThreadPool(32, new DaemonThreadFactory("job-runner-")), new InternalStepRunner(controller)); } public JobRunner(Controller controller, Duration duration, ExecutorService executors, StepRunner runner) { - this(controller, duration, executors, runner, new Metrics(controller.metric(), Duration.ofMillis(100))); - } - - JobRunner(Controller controller, Duration duration, ExecutorService executors, StepRunner runner, Metrics metrics) { super(controller, duration); this.jobs = controller.jobController(); this.jobs.setRunner(this::advance); this.executors = executors; this.runner = runner; - this.metrics = metrics; } @Override protected double maintain() { - execute(() -> jobs.active().forEach(this::advance)); + executors.execute(() -> jobs.active().forEach(this::advance)); jobs.collectGarbage(); return 1.0; } @@ -67,7 +55,6 @@ public class JobRunner extends ControllerMaintainer { @Override public void shutdown() { super.shutdown(); - metrics.shutdown(); executors.shutdown(); } @@ -96,14 +83,14 @@ public class JobRunner extends ControllerMaintainer { jobs.locked(id, run -> { if ( ! run.hasFailed() && controller().clock().instant().isAfter(run.sleepUntil().orElse(run.start()).plus(jobTimeout))) - execute(() -> { + executors.execute(() -> { jobs.abort(run.id(), "job timeout of " + jobTimeout + " reached", false); advance(run.id()); }); else if (run.readySteps().isEmpty()) - execute(() -> finish(run.id())); + executors.execute(() -> finish(run.id())); else if (run.hasFailed() || run.sleepUntil().map(sleepUntil -> ! sleepUntil.isAfter(controller().clock().instant())).orElse(true)) - run.readySteps().forEach(step -> execute(() -> advance(run.id(), step))); + run.readySteps().forEach(step -> executors.execute(() -> advance(run.id(), step))); return null; }); @@ -158,39 +145,4 @@ public class JobRunner extends ControllerMaintainer { } } - private void execute(Runnable task) { - metrics.queued.incrementAndGet(); - executors.execute(() -> { - metrics.queued.decrementAndGet(); - metrics.active.incrementAndGet(); - try { task.run(); } - finally { metrics.active.decrementAndGet(); } - }); - } - - static class Metrics { - - private final AtomicInteger queued = new AtomicInteger(); - private final AtomicInteger active = new AtomicInteger(); - private final ScheduledExecutorService reporter = Executors.newSingleThreadScheduledExecutor(new DaemonThreadFactory("job-runner-metrics-")); - private final Metric metric; - private final Metric.Context context; - - Metrics(Metric metric, Duration interval) { - this.metric = metric; - this.context = metric.createContext(Map.of()); - reporter.scheduleAtFixedRate(this::report, interval.toMillis(), interval.toMillis(), TimeUnit.MILLISECONDS); - } - - void report() { - metric.set(ControllerMetrics.DEPLOYMENT_JOBS_QUEUED.baseName(), queued.get(), context); - metric.set(ControllerMetrics.DEPLOYMENT_JOBS_ACTIVE.baseName(), active.get(), context); - } - - void shutdown() { - reporter.shutdown(); - } - - } - } diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentTester.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentTester.java index c16234b3948..1cc549ec6ca 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentTester.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentTester.java @@ -75,7 +75,8 @@ public class DeploymentTester { tester = controllerTester; jobs = tester.controller().jobController(); cloud = (MockTesterCloud) tester.controller().jobController().cloud(); - runner = new JobRunner(tester.controller(), maintenanceInterval, JobRunnerTest.inThreadExecutor(), new InternalStepRunner(tester.controller())); + runner = new JobRunner(tester.controller(), maintenanceInterval, JobRunnerTest.inThreadExecutor(), + new InternalStepRunner(tester.controller())); upgrader = new Upgrader(tester.controller(), maintenanceInterval); upgrader.setUpgradesPerMinute(1); // Anything that makes it at least one for any maintenance period is fine. readyJobsTrigger = new ReadyJobsTrigger(tester.controller(), maintenanceInterval); diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/JobRunnerTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/JobRunnerTest.java index 53be33fdfd7..e87d4f1f3f0 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/JobRunnerTest.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/JobRunnerTest.java @@ -1,10 +1,8 @@ // Copyright Vespa.ai. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. package com.yahoo.vespa.hosted.controller.maintenance; -import ai.vespa.metrics.ControllerMetrics; import com.yahoo.component.Version; import com.yahoo.config.provision.ApplicationId; -import com.yahoo.jdisc.test.MockMetric; import com.yahoo.vespa.hosted.controller.api.integration.deployment.JobId; import com.yahoo.vespa.hosted.controller.api.integration.deployment.JobType; import com.yahoo.vespa.hosted.controller.api.integration.deployment.RevisionId; @@ -24,7 +22,6 @@ import com.yahoo.vespa.hosted.controller.deployment.StepRunner; import com.yahoo.vespa.hosted.controller.deployment.Submission; import com.yahoo.vespa.hosted.controller.deployment.Versions; import com.yahoo.vespa.hosted.controller.integration.MetricsMock; -import com.yahoo.vespa.hosted.controller.maintenance.JobRunner.Metrics; import org.junit.jupiter.api.Test; import java.time.Duration; @@ -40,9 +37,7 @@ import java.util.concurrent.BrokenBarrierException; import java.util.concurrent.CyclicBarrier; import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; -import java.util.concurrent.LinkedBlockingQueue; import java.util.concurrent.Phaser; -import java.util.concurrent.ThreadPoolExecutor; import java.util.concurrent.TimeUnit; import java.util.concurrent.TimeoutException; import java.util.concurrent.atomic.AtomicBoolean; @@ -125,60 +120,6 @@ public class JobRunnerTest { assertTrue(jobs.last(id, stagingTest).get().hasEnded()); } - @Test - void metrics() { - Phaser phaser = new Phaser(4); - StepRunner runner = (step, id) -> { - phaser.arrive(); - phaser.arriveAndAwaitAdvance(); - return Optional.of(running); - }; - ExecutorService executor = new ThreadPoolExecutor(3, 3, 0, TimeUnit.SECONDS, new LinkedBlockingQueue<>(), (task, pool) -> task.run()); - DeploymentTester tester = new DeploymentTester(); - MockMetric metric = new MockMetric(); - Metrics metrics = new Metrics(metric, Duration.ofDays(1)); - JobRunner jobs = new JobRunner(tester.controller(), Duration.ofDays(1), executor, runner, metrics); - tester.newDeploymentContext().submit(); - - assertEquals(Map.of(), metric.metrics()); - metrics.report(); - assertEquals(Map.of(ControllerMetrics.DEPLOYMENT_JOBS_QUEUED.baseName(), - Map.of(Map.of(), 0.0), - ControllerMetrics.DEPLOYMENT_JOBS_ACTIVE.baseName(), - Map.of(Map.of(), 0.0)), - metric.metrics()); - tester.triggerJobs(); - - assertEquals(2, tester.jobs().active().size()); - jobs.maintain(); - phaser.arriveAndAwaitAdvance(); - metrics.report(); - assertEquals(Map.of(ControllerMetrics.DEPLOYMENT_JOBS_QUEUED.baseName(), - Map.of(Map.of(), 1.0), - ControllerMetrics.DEPLOYMENT_JOBS_ACTIVE.baseName(), - Map.of(Map.of(), 3.0)), - metric.metrics()); - - phaser.arrive(); - phaser.arriveAndAwaitAdvance(); - metrics.report(); - assertEquals(Map.of(ControllerMetrics.DEPLOYMENT_JOBS_QUEUED.baseName(), - Map.of(Map.of(), 1.0), - ControllerMetrics.DEPLOYMENT_JOBS_ACTIVE.baseName(), - Map.of(Map.of(), 3.0)), - metric.metrics()); - - jobs.shutdown(); - phaser.forceTermination(); - jobs.awaitShutdown(); - metrics.report(); - assertEquals(Map.of(ControllerMetrics.DEPLOYMENT_JOBS_QUEUED.baseName(), - Map.of(Map.of(), 0.0), - ControllerMetrics.DEPLOYMENT_JOBS_ACTIVE.baseName(), - Map.of(Map.of(), 0.0)), - metric.metrics()); - } - @Test void stepLogic() { DeploymentTester tester = new DeploymentTester(); diff --git a/documentapi/src/main/java/com/yahoo/documentapi/messagebus/protocol/AdaptiveLoadBalancer.java b/documentapi/src/main/java/com/yahoo/documentapi/messagebus/protocol/AdaptiveLoadBalancer.java index ae934857e2c..4a4cf0fd5c8 100644 --- a/documentapi/src/main/java/com/yahoo/documentapi/messagebus/protocol/AdaptiveLoadBalancer.java +++ b/documentapi/src/main/java/com/yahoo/documentapi/messagebus/protocol/AdaptiveLoadBalancer.java @@ -7,7 +7,7 @@ import java.util.List; import java.util.Random; /** - * Will pick 2 random candidates and select the one with the least pending operations. + * Will pick 2 random candidates and select the one with least pending operations. * * @author baldersheim */ diff --git a/documentapi/src/main/java/com/yahoo/documentapi/messagebus/protocol/LocalServicePolicy.java b/documentapi/src/main/java/com/yahoo/documentapi/messagebus/protocol/LocalServicePolicy.java index 4f8227b35a0..ddd04a3ca53 100755 --- a/documentapi/src/main/java/com/yahoo/documentapi/messagebus/protocol/LocalServicePolicy.java +++ b/documentapi/src/main/java/com/yahoo/documentapi/messagebus/protocol/LocalServicePolicy.java @@ -23,7 +23,7 @@ public class LocalServicePolicy implements DocumentProtocolRoutingPolicy { private final Map cache = new HashMap<>(); /** - * Constructs a policy that will choose local services that match the slobrok pattern in which this policy occurred. + * Constructs a policy that will choose local services that match the slobrok pattern in which this policy occured. * If no local service can be found, this policy simply returns the asterisk to allow the network to choose any. * * @param param The address to use for this, if empty this will resolve to hostname. diff --git a/messagebus/src/main/java/com/yahoo/messagebus/ErrorCode.java b/messagebus/src/main/java/com/yahoo/messagebus/ErrorCode.java index 88e3e1a89bc..8fab523d4ae 100644 --- a/messagebus/src/main/java/com/yahoo/messagebus/ErrorCode.java +++ b/messagebus/src/main/java/com/yahoo/messagebus/ErrorCode.java @@ -20,7 +20,7 @@ public final class ErrorCode { /** No addresses found for the services of the message route. */ public static final int NO_ADDRESS_FOR_SERVICE = TRANSIENT_ERROR + 2; - /** A connection problem occurred while sending. */ + /** A connection problem occured while sending. */ public static final int CONNECTION_ERROR = TRANSIENT_ERROR + 3; /** The session specified for the message is unknown. */ @@ -50,10 +50,10 @@ public final class ErrorCode { /** No services found for the message route. */ public static final int NO_SERVICES_FOR_ROUTE = FATAL_ERROR + 3; - /** An error occurred while encoding the message. */ + /** An error occured while encoding the message. */ public static final int ENCODE_ERROR = FATAL_ERROR + 5; - /** A fatal network error occurred while sending. */ + /** A fatal network error occured while sending. */ public static final int NETWORK_ERROR = FATAL_ERROR + 6; /** The protocol specified for the message is unknown. */ @@ -77,7 +77,7 @@ public final class ErrorCode { /** Exception thrown by routing policy. */ public static final int POLICY_ERROR = FATAL_ERROR + 13; - /** An error occurred while sequencing a message. */ + /** An error occured while sequencing a message. */ public static final int SEQUENCE_ERROR = FATAL_ERROR + 14; /** An application specific non-recoverable error. */ diff --git a/messagebus/src/main/java/com/yahoo/messagebus/routing/RoutingContext.java b/messagebus/src/main/java/com/yahoo/messagebus/routing/RoutingContext.java index 18b5de34bb4..227dd546ad8 100755 --- a/messagebus/src/main/java/com/yahoo/messagebus/routing/RoutingContext.java +++ b/messagebus/src/main/java/com/yahoo/messagebus/routing/RoutingContext.java @@ -19,7 +19,7 @@ public class RoutingContext { private final RoutingNode node; private final int directive; - private final Set consumableErrors = new HashSet<>(); + private final Set consumableErrors = new HashSet(); private boolean selectOnRetry = true; private Object context = null; diff --git a/metrics/src/main/java/ai/vespa/metrics/ControllerMetrics.java b/metrics/src/main/java/ai/vespa/metrics/ControllerMetrics.java index f03c54aa822..3676be90cd4 100644 --- a/metrics/src/main/java/ai/vespa/metrics/ControllerMetrics.java +++ b/metrics/src/main/java/ai/vespa/metrics/ControllerMetrics.java @@ -9,8 +9,6 @@ public enum ControllerMetrics implements VespaMetrics { ATHENZ_REQUEST_ERROR("athenz.request.error", Unit.REQUEST, "Controller: Athenz request error"), ARCHIVE_BUCKET_COUNT("archive.bucketCount", Unit.BUCKET, "Controller: Archive bucket count"), - DEPLOYMENT_JOBS_QUEUED("deployment.jobsQueued", Unit.TASK, "The number of deployment jobs queued"), - DEPLOYMENT_JOBS_ACTIVE("deployment.jobsActive", Unit.TASK, "The number of deployment jobs active"), DEPLOYMENT_START("deployment.start", Unit.DEPLOYMENT, "The number of started deployment jobs"), DEPLOYMENT_NODE_ALLOCATION_FAILURE("deployment.nodeAllocationFailure", Unit.DEPLOYMENT, "The number of deployments failed due to node allocation failures"), DEPLOYMENT_ENDPOINT_CERTIFICATE_TIMEOUT("deployment.endpointCertificateTimeout", Unit.DEPLOYMENT, "The number of deployments failed due to timeout acquiring endpoint certificate"), diff --git a/metrics/src/main/java/ai/vespa/metrics/set/InfrastructureMetricSet.java b/metrics/src/main/java/ai/vespa/metrics/set/InfrastructureMetricSet.java index 9443a08e28b..6bffddb885a 100644 --- a/metrics/src/main/java/ai/vespa/metrics/set/InfrastructureMetricSet.java +++ b/metrics/src/main/java/ai/vespa/metrics/set/InfrastructureMetricSet.java @@ -143,8 +143,6 @@ public class InfrastructureMetricSet { addMetric(metrics, ControllerMetrics.ARCHIVE_BUCKET_COUNT.max()); addMetric(metrics, ControllerMetrics.BILLING_TENANTS.max()); - addMetric(metrics, ControllerMetrics.DEPLOYMENT_JOBS_QUEUED, EnumSet.of(count, sum)); - addMetric(metrics, ControllerMetrics.DEPLOYMENT_JOBS_ACTIVE, EnumSet.of(count, sum)); addMetric(metrics, ControllerMetrics.DEPLOYMENT_ABORT.count()); addMetric(metrics, ControllerMetrics.DEPLOYMENT_AVERAGE_DURATION.max()); addMetric(metrics, ControllerMetrics.DEPLOYMENT_CONVERGENCE_FAILURE.count()); -- cgit v1.2.3 From 894738eb2e0a88a8868c6abaa692da176ef83969 Mon Sep 17 00:00:00 2001 From: jonmv Date: Wed, 11 Oct 2023 21:45:53 +0200 Subject: Revert "Merge pull request #28879 from vespa-engine/revert-28869-jonmv/job-runner-thread-metrics" This reverts commit 67351aa3e2adbbb4872097ed799f1ca837f35e6d, reversing changes made to aed7902ee0371efb89747d467c4a2f8124ddc08d. --- .../threadpool/ContainerThreadpoolImpl.java | 2 +- .../WorkerCompletionTimingThreadPoolExecutor.java | 3 +- .../yahoo/container/protect/ProcessTerminator.java | 4 +- .../controller/deployment/InternalStepRunner.java | 2 + .../hosted/controller/maintenance/JobRunner.java | 58 +++++++++++++++++++-- .../controller/deployment/DeploymentTester.java | 3 +- .../controller/maintenance/JobRunnerTest.java | 59 ++++++++++++++++++++++ .../messagebus/protocol/AdaptiveLoadBalancer.java | 2 +- .../messagebus/protocol/LocalServicePolicy.java | 2 +- .../main/java/com/yahoo/messagebus/ErrorCode.java | 8 +-- .../yahoo/messagebus/routing/RoutingContext.java | 2 +- .../java/ai/vespa/metrics/ControllerMetrics.java | 2 + .../vespa/metrics/set/InfrastructureMetricSet.java | 2 + 13 files changed, 130 insertions(+), 19 deletions(-) (limited to 'controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentTester.java') diff --git a/container-core/src/main/java/com/yahoo/container/handler/threadpool/ContainerThreadpoolImpl.java b/container-core/src/main/java/com/yahoo/container/handler/threadpool/ContainerThreadpoolImpl.java index befbda28ac0..f92d218390f 100644 --- a/container-core/src/main/java/com/yahoo/container/handler/threadpool/ContainerThreadpoolImpl.java +++ b/container-core/src/main/java/com/yahoo/container/handler/threadpool/ContainerThreadpoolImpl.java @@ -54,7 +54,7 @@ public class ContainerThreadpoolImpl extends AbstractComponent implements AutoCl createQueue(queueSize), ThreadFactoryFactory.getThreadFactory(name), threadPoolMetric); - // Prestart needed, if not all threads will be created by the fist N tasks and hence they might also + // Pre-start needed, if not all threads will be created by the fist N tasks and hence they might also // get the dreaded thread locals initialized even if they will never run. // That counters what we want to achieve with the Q that will prefer thread locality. executor.prestartAllCoreThreads(); diff --git a/container-core/src/main/java/com/yahoo/container/handler/threadpool/WorkerCompletionTimingThreadPoolExecutor.java b/container-core/src/main/java/com/yahoo/container/handler/threadpool/WorkerCompletionTimingThreadPoolExecutor.java index e3c2c78abec..cee2cc54b5b 100644 --- a/container-core/src/main/java/com/yahoo/container/handler/threadpool/WorkerCompletionTimingThreadPoolExecutor.java +++ b/container-core/src/main/java/com/yahoo/container/handler/threadpool/WorkerCompletionTimingThreadPoolExecutor.java @@ -8,8 +8,7 @@ import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicLong; /** - * A thread pool executor which maintains the last time a worker completed - * package private for testing + * A thread pool executor which maintains the last time a worker completed. * * @author Steinar Knutsen * @author baldersheim diff --git a/container-core/src/main/java/com/yahoo/container/protect/ProcessTerminator.java b/container-core/src/main/java/com/yahoo/container/protect/ProcessTerminator.java index 315dc21ec38..3c44ba2eaa6 100644 --- a/container-core/src/main/java/com/yahoo/container/protect/ProcessTerminator.java +++ b/container-core/src/main/java/com/yahoo/container/protect/ProcessTerminator.java @@ -4,8 +4,8 @@ package com.yahoo.container.protect; import com.yahoo.protect.Process; /** - * An injectable terminator of the Java vm. - * Components that encounters conditions where the vm should be terminated should + * An injectable terminator of the Java VM. + * Components that encounter conditions where the VM should be terminated should * request an instance of this injected. That makes termination testable * as tests can create subclasses of this which register the termination request * rather than terminating. diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/InternalStepRunner.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/InternalStepRunner.java index 662b4018a34..1080b379c4d 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/InternalStepRunner.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/deployment/InternalStepRunner.java @@ -727,6 +727,8 @@ public class InternalStepRunner implements StepRunner { DeploymentSpec spec = controller.applications().requireApplication(TenantAndApplicationId.from(id.application())).deploymentSpec(); boolean requireTests = spec.steps().stream().anyMatch(step -> step.concerns(id.type().environment())); + logger.log(WARNING, "No tests were actually run, but this test suite is explicitly declared in 'deployment.xml'. " + + "Either add tests, ensure they're correctly configured, or remove the test declaration."); return Optional.of(requireTests ? testFailure : noTests); } case SUCCESS: diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/JobRunner.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/JobRunner.java index 6f00ff39637..0f482b1a015 100644 --- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/JobRunner.java +++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/JobRunner.java @@ -1,7 +1,9 @@ // Copyright Vespa.ai. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. package com.yahoo.vespa.hosted.controller.maintenance; +import ai.vespa.metrics.ControllerMetrics; import com.yahoo.concurrent.DaemonThreadFactory; +import com.yahoo.jdisc.Metric; import com.yahoo.vespa.hosted.controller.Controller; import com.yahoo.vespa.hosted.controller.api.integration.deployment.RunId; import com.yahoo.vespa.hosted.controller.deployment.InternalStepRunner; @@ -11,11 +13,14 @@ import com.yahoo.vespa.hosted.controller.deployment.Step; import com.yahoo.vespa.hosted.controller.deployment.StepRunner; import java.time.Duration; +import java.util.Map; import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; +import java.util.concurrent.ScheduledExecutorService; import java.util.concurrent.TimeUnit; import java.util.concurrent.TimeoutException; import java.util.concurrent.atomic.AtomicBoolean; +import java.util.concurrent.atomic.AtomicInteger; import java.util.logging.Level; import java.util.logging.Logger; @@ -32,22 +37,29 @@ public class JobRunner extends ControllerMaintainer { private final JobController jobs; private final ExecutorService executors; private final StepRunner runner; + private final Metrics metrics; public JobRunner(Controller controller, Duration duration) { - this(controller, duration, Executors.newFixedThreadPool(32, new DaemonThreadFactory("job-runner-")), new InternalStepRunner(controller)); + this(controller, duration, Executors.newFixedThreadPool(32, new DaemonThreadFactory("job-runner-")), + new InternalStepRunner(controller)); } public JobRunner(Controller controller, Duration duration, ExecutorService executors, StepRunner runner) { + this(controller, duration, executors, runner, new Metrics(controller.metric(), Duration.ofMillis(100))); + } + + JobRunner(Controller controller, Duration duration, ExecutorService executors, StepRunner runner, Metrics metrics) { super(controller, duration); this.jobs = controller.jobController(); this.jobs.setRunner(this::advance); this.executors = executors; this.runner = runner; + this.metrics = metrics; } @Override protected double maintain() { - executors.execute(() -> jobs.active().forEach(this::advance)); + execute(() -> jobs.active().forEach(this::advance)); jobs.collectGarbage(); return 1.0; } @@ -55,6 +67,7 @@ public class JobRunner extends ControllerMaintainer { @Override public void shutdown() { super.shutdown(); + metrics.shutdown(); executors.shutdown(); } @@ -83,14 +96,14 @@ public class JobRunner extends ControllerMaintainer { jobs.locked(id, run -> { if ( ! run.hasFailed() && controller().clock().instant().isAfter(run.sleepUntil().orElse(run.start()).plus(jobTimeout))) - executors.execute(() -> { + execute(() -> { jobs.abort(run.id(), "job timeout of " + jobTimeout + " reached", false); advance(run.id()); }); else if (run.readySteps().isEmpty()) - executors.execute(() -> finish(run.id())); + execute(() -> finish(run.id())); else if (run.hasFailed() || run.sleepUntil().map(sleepUntil -> ! sleepUntil.isAfter(controller().clock().instant())).orElse(true)) - run.readySteps().forEach(step -> executors.execute(() -> advance(run.id(), step))); + run.readySteps().forEach(step -> execute(() -> advance(run.id(), step))); return null; }); @@ -145,4 +158,39 @@ public class JobRunner extends ControllerMaintainer { } } + private void execute(Runnable task) { + metrics.queued.incrementAndGet(); + executors.execute(() -> { + metrics.queued.decrementAndGet(); + metrics.active.incrementAndGet(); + try { task.run(); } + finally { metrics.active.decrementAndGet(); } + }); + } + + static class Metrics { + + private final AtomicInteger queued = new AtomicInteger(); + private final AtomicInteger active = new AtomicInteger(); + private final ScheduledExecutorService reporter = Executors.newSingleThreadScheduledExecutor(new DaemonThreadFactory("job-runner-metrics-")); + private final Metric metric; + private final Metric.Context context; + + Metrics(Metric metric, Duration interval) { + this.metric = metric; + this.context = metric.createContext(Map.of()); + reporter.scheduleAtFixedRate(this::report, interval.toMillis(), interval.toMillis(), TimeUnit.MILLISECONDS); + } + + void report() { + metric.set(ControllerMetrics.DEPLOYMENT_JOBS_QUEUED.baseName(), queued.get(), context); + metric.set(ControllerMetrics.DEPLOYMENT_JOBS_ACTIVE.baseName(), active.get(), context); + } + + void shutdown() { + reporter.shutdown(); + } + + } + } diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentTester.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentTester.java index 1cc549ec6ca..c16234b3948 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentTester.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentTester.java @@ -75,8 +75,7 @@ public class DeploymentTester { tester = controllerTester; jobs = tester.controller().jobController(); cloud = (MockTesterCloud) tester.controller().jobController().cloud(); - runner = new JobRunner(tester.controller(), maintenanceInterval, JobRunnerTest.inThreadExecutor(), - new InternalStepRunner(tester.controller())); + runner = new JobRunner(tester.controller(), maintenanceInterval, JobRunnerTest.inThreadExecutor(), new InternalStepRunner(tester.controller())); upgrader = new Upgrader(tester.controller(), maintenanceInterval); upgrader.setUpgradesPerMinute(1); // Anything that makes it at least one for any maintenance period is fine. readyJobsTrigger = new ReadyJobsTrigger(tester.controller(), maintenanceInterval); diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/JobRunnerTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/JobRunnerTest.java index e87d4f1f3f0..53be33fdfd7 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/JobRunnerTest.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/JobRunnerTest.java @@ -1,8 +1,10 @@ // Copyright Vespa.ai. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. package com.yahoo.vespa.hosted.controller.maintenance; +import ai.vespa.metrics.ControllerMetrics; import com.yahoo.component.Version; import com.yahoo.config.provision.ApplicationId; +import com.yahoo.jdisc.test.MockMetric; import com.yahoo.vespa.hosted.controller.api.integration.deployment.JobId; import com.yahoo.vespa.hosted.controller.api.integration.deployment.JobType; import com.yahoo.vespa.hosted.controller.api.integration.deployment.RevisionId; @@ -22,6 +24,7 @@ import com.yahoo.vespa.hosted.controller.deployment.StepRunner; import com.yahoo.vespa.hosted.controller.deployment.Submission; import com.yahoo.vespa.hosted.controller.deployment.Versions; import com.yahoo.vespa.hosted.controller.integration.MetricsMock; +import com.yahoo.vespa.hosted.controller.maintenance.JobRunner.Metrics; import org.junit.jupiter.api.Test; import java.time.Duration; @@ -37,7 +40,9 @@ import java.util.concurrent.BrokenBarrierException; import java.util.concurrent.CyclicBarrier; import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; +import java.util.concurrent.LinkedBlockingQueue; import java.util.concurrent.Phaser; +import java.util.concurrent.ThreadPoolExecutor; import java.util.concurrent.TimeUnit; import java.util.concurrent.TimeoutException; import java.util.concurrent.atomic.AtomicBoolean; @@ -120,6 +125,60 @@ public class JobRunnerTest { assertTrue(jobs.last(id, stagingTest).get().hasEnded()); } + @Test + void metrics() { + Phaser phaser = new Phaser(4); + StepRunner runner = (step, id) -> { + phaser.arrive(); + phaser.arriveAndAwaitAdvance(); + return Optional.of(running); + }; + ExecutorService executor = new ThreadPoolExecutor(3, 3, 0, TimeUnit.SECONDS, new LinkedBlockingQueue<>(), (task, pool) -> task.run()); + DeploymentTester tester = new DeploymentTester(); + MockMetric metric = new MockMetric(); + Metrics metrics = new Metrics(metric, Duration.ofDays(1)); + JobRunner jobs = new JobRunner(tester.controller(), Duration.ofDays(1), executor, runner, metrics); + tester.newDeploymentContext().submit(); + + assertEquals(Map.of(), metric.metrics()); + metrics.report(); + assertEquals(Map.of(ControllerMetrics.DEPLOYMENT_JOBS_QUEUED.baseName(), + Map.of(Map.of(), 0.0), + ControllerMetrics.DEPLOYMENT_JOBS_ACTIVE.baseName(), + Map.of(Map.of(), 0.0)), + metric.metrics()); + tester.triggerJobs(); + + assertEquals(2, tester.jobs().active().size()); + jobs.maintain(); + phaser.arriveAndAwaitAdvance(); + metrics.report(); + assertEquals(Map.of(ControllerMetrics.DEPLOYMENT_JOBS_QUEUED.baseName(), + Map.of(Map.of(), 1.0), + ControllerMetrics.DEPLOYMENT_JOBS_ACTIVE.baseName(), + Map.of(Map.of(), 3.0)), + metric.metrics()); + + phaser.arrive(); + phaser.arriveAndAwaitAdvance(); + metrics.report(); + assertEquals(Map.of(ControllerMetrics.DEPLOYMENT_JOBS_QUEUED.baseName(), + Map.of(Map.of(), 1.0), + ControllerMetrics.DEPLOYMENT_JOBS_ACTIVE.baseName(), + Map.of(Map.of(), 3.0)), + metric.metrics()); + + jobs.shutdown(); + phaser.forceTermination(); + jobs.awaitShutdown(); + metrics.report(); + assertEquals(Map.of(ControllerMetrics.DEPLOYMENT_JOBS_QUEUED.baseName(), + Map.of(Map.of(), 0.0), + ControllerMetrics.DEPLOYMENT_JOBS_ACTIVE.baseName(), + Map.of(Map.of(), 0.0)), + metric.metrics()); + } + @Test void stepLogic() { DeploymentTester tester = new DeploymentTester(); diff --git a/documentapi/src/main/java/com/yahoo/documentapi/messagebus/protocol/AdaptiveLoadBalancer.java b/documentapi/src/main/java/com/yahoo/documentapi/messagebus/protocol/AdaptiveLoadBalancer.java index 4a4cf0fd5c8..ae934857e2c 100644 --- a/documentapi/src/main/java/com/yahoo/documentapi/messagebus/protocol/AdaptiveLoadBalancer.java +++ b/documentapi/src/main/java/com/yahoo/documentapi/messagebus/protocol/AdaptiveLoadBalancer.java @@ -7,7 +7,7 @@ import java.util.List; import java.util.Random; /** - * Will pick 2 random candidates and select the one with least pending operations. + * Will pick 2 random candidates and select the one with the least pending operations. * * @author baldersheim */ diff --git a/documentapi/src/main/java/com/yahoo/documentapi/messagebus/protocol/LocalServicePolicy.java b/documentapi/src/main/java/com/yahoo/documentapi/messagebus/protocol/LocalServicePolicy.java index ddd04a3ca53..4f8227b35a0 100755 --- a/documentapi/src/main/java/com/yahoo/documentapi/messagebus/protocol/LocalServicePolicy.java +++ b/documentapi/src/main/java/com/yahoo/documentapi/messagebus/protocol/LocalServicePolicy.java @@ -23,7 +23,7 @@ public class LocalServicePolicy implements DocumentProtocolRoutingPolicy { private final Map cache = new HashMap<>(); /** - * Constructs a policy that will choose local services that match the slobrok pattern in which this policy occured. + * Constructs a policy that will choose local services that match the slobrok pattern in which this policy occurred. * If no local service can be found, this policy simply returns the asterisk to allow the network to choose any. * * @param param The address to use for this, if empty this will resolve to hostname. diff --git a/messagebus/src/main/java/com/yahoo/messagebus/ErrorCode.java b/messagebus/src/main/java/com/yahoo/messagebus/ErrorCode.java index 8fab523d4ae..88e3e1a89bc 100644 --- a/messagebus/src/main/java/com/yahoo/messagebus/ErrorCode.java +++ b/messagebus/src/main/java/com/yahoo/messagebus/ErrorCode.java @@ -20,7 +20,7 @@ public final class ErrorCode { /** No addresses found for the services of the message route. */ public static final int NO_ADDRESS_FOR_SERVICE = TRANSIENT_ERROR + 2; - /** A connection problem occured while sending. */ + /** A connection problem occurred while sending. */ public static final int CONNECTION_ERROR = TRANSIENT_ERROR + 3; /** The session specified for the message is unknown. */ @@ -50,10 +50,10 @@ public final class ErrorCode { /** No services found for the message route. */ public static final int NO_SERVICES_FOR_ROUTE = FATAL_ERROR + 3; - /** An error occured while encoding the message. */ + /** An error occurred while encoding the message. */ public static final int ENCODE_ERROR = FATAL_ERROR + 5; - /** A fatal network error occured while sending. */ + /** A fatal network error occurred while sending. */ public static final int NETWORK_ERROR = FATAL_ERROR + 6; /** The protocol specified for the message is unknown. */ @@ -77,7 +77,7 @@ public final class ErrorCode { /** Exception thrown by routing policy. */ public static final int POLICY_ERROR = FATAL_ERROR + 13; - /** An error occured while sequencing a message. */ + /** An error occurred while sequencing a message. */ public static final int SEQUENCE_ERROR = FATAL_ERROR + 14; /** An application specific non-recoverable error. */ diff --git a/messagebus/src/main/java/com/yahoo/messagebus/routing/RoutingContext.java b/messagebus/src/main/java/com/yahoo/messagebus/routing/RoutingContext.java index 227dd546ad8..18b5de34bb4 100755 --- a/messagebus/src/main/java/com/yahoo/messagebus/routing/RoutingContext.java +++ b/messagebus/src/main/java/com/yahoo/messagebus/routing/RoutingContext.java @@ -19,7 +19,7 @@ public class RoutingContext { private final RoutingNode node; private final int directive; - private final Set consumableErrors = new HashSet(); + private final Set consumableErrors = new HashSet<>(); private boolean selectOnRetry = true; private Object context = null; diff --git a/metrics/src/main/java/ai/vespa/metrics/ControllerMetrics.java b/metrics/src/main/java/ai/vespa/metrics/ControllerMetrics.java index 3676be90cd4..f03c54aa822 100644 --- a/metrics/src/main/java/ai/vespa/metrics/ControllerMetrics.java +++ b/metrics/src/main/java/ai/vespa/metrics/ControllerMetrics.java @@ -9,6 +9,8 @@ public enum ControllerMetrics implements VespaMetrics { ATHENZ_REQUEST_ERROR("athenz.request.error", Unit.REQUEST, "Controller: Athenz request error"), ARCHIVE_BUCKET_COUNT("archive.bucketCount", Unit.BUCKET, "Controller: Archive bucket count"), + DEPLOYMENT_JOBS_QUEUED("deployment.jobsQueued", Unit.TASK, "The number of deployment jobs queued"), + DEPLOYMENT_JOBS_ACTIVE("deployment.jobsActive", Unit.TASK, "The number of deployment jobs active"), DEPLOYMENT_START("deployment.start", Unit.DEPLOYMENT, "The number of started deployment jobs"), DEPLOYMENT_NODE_ALLOCATION_FAILURE("deployment.nodeAllocationFailure", Unit.DEPLOYMENT, "The number of deployments failed due to node allocation failures"), DEPLOYMENT_ENDPOINT_CERTIFICATE_TIMEOUT("deployment.endpointCertificateTimeout", Unit.DEPLOYMENT, "The number of deployments failed due to timeout acquiring endpoint certificate"), diff --git a/metrics/src/main/java/ai/vespa/metrics/set/InfrastructureMetricSet.java b/metrics/src/main/java/ai/vespa/metrics/set/InfrastructureMetricSet.java index 6bffddb885a..9443a08e28b 100644 --- a/metrics/src/main/java/ai/vespa/metrics/set/InfrastructureMetricSet.java +++ b/metrics/src/main/java/ai/vespa/metrics/set/InfrastructureMetricSet.java @@ -143,6 +143,8 @@ public class InfrastructureMetricSet { addMetric(metrics, ControllerMetrics.ARCHIVE_BUCKET_COUNT.max()); addMetric(metrics, ControllerMetrics.BILLING_TENANTS.max()); + addMetric(metrics, ControllerMetrics.DEPLOYMENT_JOBS_QUEUED, EnumSet.of(count, sum)); + addMetric(metrics, ControllerMetrics.DEPLOYMENT_JOBS_ACTIVE, EnumSet.of(count, sum)); addMetric(metrics, ControllerMetrics.DEPLOYMENT_ABORT.count()); addMetric(metrics, ControllerMetrics.DEPLOYMENT_AVERAGE_DURATION.max()); addMetric(metrics, ControllerMetrics.DEPLOYMENT_CONVERGENCE_FAILURE.count()); -- cgit v1.2.3 From 67710e7e80999eb97fdfe2df3873f05824ab503f Mon Sep 17 00:00:00 2001 From: jonmv Date: Thu, 19 Oct 2023 09:53:54 +0200 Subject: Use BFS rather than DFS task order for in-thread test executor --- .../controller/deployment/DeploymentTester.java | 2 +- .../controller/maintenance/JobRunnerTest.java | 63 +++++++++++++++++++--- 2 files changed, 56 insertions(+), 9 deletions(-) (limited to 'controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentTester.java') diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentTester.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentTester.java index c16234b3948..bc03d46a30a 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentTester.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/deployment/DeploymentTester.java @@ -75,7 +75,7 @@ public class DeploymentTester { tester = controllerTester; jobs = tester.controller().jobController(); cloud = (MockTesterCloud) tester.controller().jobController().cloud(); - runner = new JobRunner(tester.controller(), maintenanceInterval, JobRunnerTest.inThreadExecutor(), new InternalStepRunner(tester.controller())); + runner = new JobRunner(tester.controller(), maintenanceInterval, JobRunnerTest.inThreadInOrderExecutor(), new InternalStepRunner(tester.controller())); upgrader = new Upgrader(tester.controller(), maintenanceInterval); upgrader.setUpgradesPerMinute(1); // Anything that makes it at least one for any maintenance period is fine. readyJobsTrigger = new ReadyJobsTrigger(tester.controller(), maintenanceInterval); diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/JobRunnerTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/JobRunnerTest.java index 20717be598f..d96de8df6fd 100644 --- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/JobRunnerTest.java +++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/maintenance/JobRunnerTest.java @@ -34,18 +34,24 @@ import java.util.EnumMap; import java.util.List; import java.util.Map; import java.util.Optional; +import java.util.Queue; import java.util.Set; import java.util.concurrent.AbstractExecutorService; import java.util.concurrent.BrokenBarrierException; +import java.util.concurrent.ConcurrentLinkedQueue; import java.util.concurrent.CyclicBarrier; +import java.util.concurrent.Executor; import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; import java.util.concurrent.LinkedBlockingQueue; import java.util.concurrent.Phaser; +import java.util.concurrent.RejectedExecutionException; import java.util.concurrent.ThreadPoolExecutor; import java.util.concurrent.TimeUnit; import java.util.concurrent.TimeoutException; import java.util.concurrent.atomic.AtomicBoolean; +import java.util.concurrent.atomic.AtomicInteger; +import java.util.function.Consumer; import java.util.function.Supplier; import java.util.stream.Collectors; @@ -70,10 +76,12 @@ import static com.yahoo.vespa.hosted.controller.deployment.Step.installReal; import static com.yahoo.vespa.hosted.controller.deployment.Step.installTester; import static com.yahoo.vespa.hosted.controller.deployment.Step.report; import static com.yahoo.vespa.hosted.controller.deployment.Step.startTests; +import static java.util.Objects.requireNonNull; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNotSame; import static org.junit.jupiter.api.Assertions.assertSame; +import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.junit.jupiter.api.Assertions.fail; @@ -175,7 +183,7 @@ public class JobRunnerTest { DeploymentTester tester = new DeploymentTester(); JobController jobs = tester.controller().jobController(); Map outcomes = new EnumMap<>(Step.class); - JobRunner runner = new JobRunner(tester.controller(), Duration.ofDays(1), inThreadExecutor(), mappedRunner(outcomes)); + JobRunner runner = new JobRunner(tester.controller(), Duration.ofDays(1), inThreadInOrderExecutor(), mappedRunner(outcomes)); TenantAndApplicationId appId = tester.createApplication("tenant", "real", "default").id(); ApplicationId id = appId.defaultInstance(); @@ -322,7 +330,7 @@ public class JobRunnerTest { void historyPruning() { DeploymentTester tester = new DeploymentTester(); JobController jobs = tester.controller().jobController(); - JobRunner runner = new JobRunner(tester.controller(), Duration.ofDays(1), inThreadExecutor(), (id, step) -> Optional.of(running)); + JobRunner runner = new JobRunner(tester.controller(), Duration.ofDays(1), inThreadInOrderExecutor(), (id, step) -> Optional.of(running)); TenantAndApplicationId appId = tester.createApplication("tenant", "real", "default").id(); ApplicationId instanceId = appId.defaultInstance(); @@ -347,7 +355,7 @@ public class JobRunnerTest { assertFalse(jobs.details(new RunId(instanceId, systemTest, 1)).isPresent()); assertTrue(jobs.details(new RunId(instanceId, systemTest, 65)).isPresent()); - JobRunner failureRunner = new JobRunner(tester.controller(), Duration.ofDays(1), inThreadExecutor(), (id, step) -> Optional.of(error)); + JobRunner failureRunner = new JobRunner(tester.controller(), Duration.ofDays(1), inThreadInOrderExecutor(), (id, step) -> Optional.of(error)); // Make all but the oldest of the 54 jobs a failure. for (int i = 0; i < jobs.historyLength() - 1; i++) { @@ -419,7 +427,7 @@ public class JobRunnerTest { DeploymentTester tester = new DeploymentTester(); JobController jobs = tester.controller().jobController(); Map outcomes = new EnumMap<>(Step.class); - JobRunner runner = new JobRunner(tester.controller(), Duration.ofDays(1), inThreadExecutor(), mappedRunner(outcomes)); + JobRunner runner = new JobRunner(tester.controller(), Duration.ofDays(1), inThreadInOrderExecutor(), mappedRunner(outcomes)); TenantAndApplicationId appId = tester.createApplication("tenant", "real", "default").id(); ApplicationId id = appId.defaultInstance(); @@ -437,7 +445,7 @@ public class JobRunnerTest { DeploymentTester tester = new DeploymentTester(); JobController jobs = tester.controller().jobController(); Map outcomes = new EnumMap<>(Step.class); - JobRunner runner = new JobRunner(tester.controller(), Duration.ofDays(1), inThreadExecutor(), mappedRunner(outcomes)); + JobRunner runner = new JobRunner(tester.controller(), Duration.ofDays(1), inThreadInOrderExecutor(), mappedRunner(outcomes)); TenantAndApplicationId appId = tester.createApplication("tenant", "real", "default").id(); ApplicationId id = appId.defaultInstance(); @@ -473,19 +481,58 @@ public class JobRunnerTest { assertEquals(1, metric.getMetric(context::equals, JobMetrics.noTests).get().intValue()); } + @Test + void testInThreadExecutor() throws InterruptedException { + ExecutorService executor = inThreadInOrderExecutor(); + AtomicInteger c = new AtomicInteger(0), d = new AtomicInteger(0); + Consumer task = i -> executor.execute(() -> { + executor.execute(() -> { + i.set(2); + executor.execute(() -> i.set(4)); + }); + executor.execute(() -> i.set(3)); + i.set(1); + }); + Thread s = new Thread(() -> task.accept(d)); + s.start(); + task.accept(c); + s.join(); + assertEquals(4, c.get()); + assertEquals(4, d.get()); + assertEquals("executor is shut down", + assertThrows(RejectedExecutionException.class, + () -> executor.execute(() -> { + executor.execute(() -> executor.execute(() -> { c.set(6); })); + executor.shutdown(); + c.set(5); + })).getMessage()); + assertEquals(5, c.get()); + } + private void start(JobController jobs, ApplicationId id, JobType type) { jobs.start(id, type, versions, false, Reason.empty()); } - public static ExecutorService inThreadExecutor() { + /** Dummy test executor for unit tests. Runs tasks BFS rather than DFS, like a simple {@code Runnable::run} would do. No real shutdown logic. */ + public static ExecutorService inThreadInOrderExecutor() { return new AbstractExecutorService() { - final AtomicBoolean shutDown = new AtomicBoolean(false); + private final ThreadLocal inExecute = ThreadLocal.withInitial(() -> false); + private final ThreadLocal> tasks = ThreadLocal.withInitial(ConcurrentLinkedQueue::new); + private final AtomicBoolean shutDown = new AtomicBoolean(false); + @Override + public void execute(Runnable command) { + if (isShutdown()) throw new RejectedExecutionException("executor is shut down"); + tasks.get().add(requireNonNull(command)); + if (inExecute.get()) return; + inExecute.set(true); + try { Runnable task; while (null != (task = tasks.get().poll())) task.run(); } + finally { inExecute.set(false); } + } @Override public void shutdown() { shutDown.set(true); } @Override public List shutdownNow() { shutDown.set(true); return Collections.emptyList(); } @Override public boolean isShutdown() { return shutDown.get(); } @Override public boolean isTerminated() { return shutDown.get(); } @Override public boolean awaitTermination(long timeout, TimeUnit unit) { return true; } - @Override public void execute(Runnable command) { command.run(); } }; } -- cgit v1.2.3