diff options
author | Harald Musum <musum@verizonmedia.com> | 2019-10-22 07:59:05 +0200 |
---|---|---|
committer | GitHub <noreply@github.com> | 2019-10-22 07:59:05 +0200 |
commit | 3889e10589b3809b69623c56434224971fb4b9c6 (patch) | |
tree | 55b5ced462425ab04497eeea55fa507f81c18767 /orchestrator/src | |
parent | 9837e9b7ff4b7541db0caede471bf418f3ded02a (diff) |
Revert "Add Orchestrator application lock metrics"
Diffstat (limited to 'orchestrator/src')
5 files changed, 13 insertions, 111 deletions
diff --git a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/status/ZookeeperStatusService.java b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/status/ZookeeperStatusService.java index 65bdaed86b8..e3d2a0827ed 100644 --- a/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/status/ZookeeperStatusService.java +++ b/orchestrator/src/main/java/com/yahoo/vespa/orchestrator/status/ZookeeperStatusService.java @@ -2,10 +2,7 @@ package com.yahoo.vespa.orchestrator.status; import com.google.common.util.concurrent.UncheckedTimeoutException; -import com.yahoo.config.provision.ApplicationId; import com.yahoo.container.jaxrs.annotation.Component; -import com.yahoo.jdisc.Metric; -import com.yahoo.jdisc.Timer; import com.yahoo.log.LogLevel; import com.yahoo.vespa.applicationmodel.ApplicationInstanceReference; import com.yahoo.vespa.applicationmodel.HostName; @@ -20,7 +17,6 @@ import org.apache.zookeeper.data.Stat; import javax.inject.Inject; import java.time.Duration; -import java.time.Instant; import java.util.Collections; import java.util.HashSet; import java.util.Map; @@ -46,27 +42,18 @@ public class ZookeeperStatusService implements StatusService { private final Curator curator; private final CuratorCounter counter; - private final Metric metric; - private final Timer timer; - - /** - * A cache of metric contexts for each possible dimension map. In practice, there is one dimension map - * for each application, so up to hundreds of elements. - */ - private final ConcurrentHashMap<Map<String, String>, Metric.Context> cachedContexts = new ConcurrentHashMap<>(); /** A cache of hosts allowed to be down. Access only through {@link #getValidCache()}! */ - private final Map<ApplicationInstanceReference, Set<HostName>> hostsDown = new ConcurrentHashMap<>(); + private final Map<ApplicationInstanceReference, Set<HostName>> hostsDown; private volatile long cacheRefreshedAt; @Inject - public ZookeeperStatusService(@Component Curator curator, @Component Metric metric, @Component Timer timer) { + public ZookeeperStatusService(@Component Curator curator) { this.curator = curator; this.counter = new CuratorCounter(curator, HOST_STATUS_CACHE_COUNTER_PATH); this.cacheRefreshedAt = counter.get(); - this.metric = metric; - this.timer = timer; + this.hostsDown = new ConcurrentHashMap<>(); } @Override @@ -117,51 +104,20 @@ public class ZookeeperStatusService implements StatusService { public MutableStatusRegistry lockApplicationInstance_forCurrentThreadOnly( OrchestratorContext context, ApplicationInstanceReference applicationInstanceReference) throws UncheckedTimeoutException { - ApplicationId applicationId = OrchestratorUtil.toApplicationId(applicationInstanceReference); - String app = applicationId.application().value() + "." + applicationId.instance().value(); - Map<String, String> dimensions = Map.of( - "tenantName", applicationId.tenant().value(), - "applicationId", applicationId.toFullString(), - "app", app); - Metric.Context metricContext = cachedContexts.computeIfAbsent(dimensions, metric::createContext); - Duration duration = context.getTimeLeft(); String lockPath = applicationInstanceLock2Path(applicationInstanceReference); Lock lock = new Lock(lockPath, curator); - - Instant startTime = timer.currentTime(); - Instant acquireEndTime; - boolean lockAcquired = false; - try { - lock.acquire(duration); - lockAcquired = true; - } finally { - acquireEndTime = timer.currentTime(); - double seconds = durationInSeconds(startTime, acquireEndTime); - metric.set("orchestrator.lock.acquire-latency", seconds, metricContext); - metric.set("orchestrator.lock.acquired", lockAcquired ? 1 : 0, metricContext); - } - - Runnable updateLockHoldMetric = () -> { - Instant lockReleasedTime = timer.currentTime(); - double seconds = durationInSeconds(acquireEndTime, lockReleasedTime); - metric.set("orchestrator.lock.hold-latency", seconds, metricContext); - }; + lock.acquire(duration); try { - return new ZkMutableStatusRegistry(lock, applicationInstanceReference, context.isProbe(), updateLockHoldMetric); + return new ZkMutableStatusRegistry(lock, applicationInstanceReference, context.isProbe()); } catch (Throwable t) { // In case the constructor throws an exception. - updateLockHoldMetric.run(); lock.close(); throw t; } } - private double durationInSeconds(Instant startInstant, Instant endInstant) { - return Duration.between(startInstant, endInstant).toMillis() / 1000.0; - } - private void setHostStatus(ApplicationInstanceReference applicationInstanceReference, HostName hostName, HostStatus status) { @@ -281,16 +237,13 @@ public class ZookeeperStatusService implements StatusService { private final Lock lock; private final ApplicationInstanceReference applicationInstanceReference; private final boolean probe; - private final Runnable onLockRelease; public ZkMutableStatusRegistry(Lock lock, ApplicationInstanceReference applicationInstanceReference, - boolean probe, - Runnable onLockRelease) { + boolean probe) { this.lock = lock; this.applicationInstanceReference = applicationInstanceReference; this.probe = probe; - this.onLockRelease = onLockRelease; } @Override @@ -340,7 +293,6 @@ public class ZookeeperStatusService implements StatusService { @Override public void close() { - onLockRelease.run(); try { lock.close(); } catch (RuntimeException e) { diff --git a/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/OrchestratorImplTest.java b/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/OrchestratorImplTest.java index 4d7a0f18918..af93d497677 100644 --- a/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/OrchestratorImplTest.java +++ b/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/OrchestratorImplTest.java @@ -2,8 +2,6 @@ package com.yahoo.vespa.orchestrator; import com.yahoo.config.provision.ApplicationId; -import com.yahoo.jdisc.Metric; -import com.yahoo.jdisc.test.TestTimer; import com.yahoo.vespa.applicationmodel.ApplicationInstance; import com.yahoo.vespa.applicationmodel.ApplicationInstanceId; import com.yahoo.vespa.applicationmodel.ApplicationInstanceReference; @@ -51,7 +49,6 @@ import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.doThrow; import static org.mockito.Mockito.inOrder; -import static org.mockito.Mockito.mock; import static org.mockito.Mockito.spy; /** @@ -80,7 +77,7 @@ public class OrchestratorImplTest { clustercontroller = new ClusterControllerClientFactoryMock(); orchestrator = new OrchestratorImpl( clustercontroller, - new ZookeeperStatusService(new MockCurator(), mock(Metric.class), new TestTimer()), + new ZookeeperStatusService(new MockCurator()), new OrchestratorConfig(new OrchestratorConfig.Builder()), new DummyInstanceLookupService()); @@ -314,7 +311,7 @@ public class OrchestratorImplTest { @Test public void testGetHost() throws Exception { ClusterControllerClientFactory clusterControllerClientFactory = new ClusterControllerClientFactoryMock(); - StatusService statusService = new ZookeeperStatusService(new MockCurator(), mock(Metric.class), new TestTimer()); + StatusService statusService = new ZookeeperStatusService(new MockCurator()); HostName hostName = new HostName("host.yahoo.com"); TenantId tenantId = new TenantId("tenant"); diff --git a/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/model/ModelTestUtils.java b/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/model/ModelTestUtils.java index 544ac27c92f..0def668e147 100644 --- a/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/model/ModelTestUtils.java +++ b/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/model/ModelTestUtils.java @@ -1,8 +1,6 @@ // Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. package com.yahoo.vespa.orchestrator.model; -import com.yahoo.jdisc.Metric; -import com.yahoo.jdisc.test.TestTimer; import com.yahoo.vespa.applicationmodel.ApplicationInstance; import com.yahoo.vespa.applicationmodel.ApplicationInstanceId; import com.yahoo.vespa.applicationmodel.ApplicationInstanceReference; @@ -38,14 +36,12 @@ import java.util.List; import java.util.Map; import java.util.Set; -import static org.mockito.Mockito.mock; - class ModelTestUtils { private final Map<ApplicationInstanceReference, ApplicationInstance> applications = new HashMap<>(); private final ClusterControllerClientFactory clusterControllerClientFactory = new ClusterControllerClientFactoryMock(); private final Map<HostName, HostStatus> hostStatusMap = new HashMap<>(); - private final StatusService statusService = new ZookeeperStatusService(new MockCurator(), mock(Metric.class), new TestTimer()); + private final StatusService statusService = new ZookeeperStatusService(new MockCurator()); private final Orchestrator orchestrator = new OrchestratorImpl(clusterControllerClientFactory, statusService, new OrchestratorConfig(new Builder()), diff --git a/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/resources/HostResourceTest.java b/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/resources/HostResourceTest.java index 32758970d75..bbc526cfbe1 100644 --- a/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/resources/HostResourceTest.java +++ b/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/resources/HostResourceTest.java @@ -2,8 +2,6 @@ package com.yahoo.vespa.orchestrator.resources; import com.google.common.util.concurrent.UncheckedTimeoutException; -import com.yahoo.jdisc.Metric; -import com.yahoo.jdisc.test.TestTimer; import com.yahoo.vespa.applicationmodel.ApplicationInstance; import com.yahoo.vespa.applicationmodel.ApplicationInstanceId; import com.yahoo.vespa.applicationmodel.ApplicationInstanceReference; @@ -74,8 +72,7 @@ public class HostResourceTest { private static final int SERVICE_MONITOR_CONVERGENCE_LATENCY_SECONDS = 0; private static final TenantId TENANT_ID = new TenantId("tenantId"); private static final ApplicationInstanceId APPLICATION_INSTANCE_ID = new ApplicationInstanceId("applicationId"); - private static final StatusService EVERY_HOST_IS_UP_HOST_STATUS_SERVICE = new ZookeeperStatusService( - new MockCurator(), mock(Metric.class), new TestTimer()); + private static final StatusService EVERY_HOST_IS_UP_HOST_STATUS_SERVICE = new ZookeeperStatusService(new MockCurator()); private static final InstanceLookupService mockInstanceLookupService = mock(InstanceLookupService.class); static { diff --git a/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/status/ZookeeperStatusServiceTest.java b/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/status/ZookeeperStatusServiceTest.java index 707c81b92a5..9b1be12121d 100644 --- a/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/status/ZookeeperStatusServiceTest.java +++ b/orchestrator/src/test/java/com/yahoo/vespa/orchestrator/status/ZookeeperStatusServiceTest.java @@ -1,9 +1,6 @@ // Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. package com.yahoo.vespa.orchestrator.status; -import com.yahoo.jdisc.Metric; -import com.yahoo.jdisc.Timer; -import com.yahoo.jdisc.test.TestTimer; import com.yahoo.log.LogLevel; import com.yahoo.vespa.applicationmodel.ApplicationInstanceReference; import com.yahoo.vespa.curator.Curator; @@ -19,18 +16,12 @@ import org.hamcrest.TypeSafeMatcher; import org.junit.After; import org.junit.Before; import org.junit.Test; -import org.junit.runner.RunWith; -import org.mockito.ArgumentCaptor; -import org.mockito.Captor; -import org.mockito.junit.MockitoJUnitRunner; import java.time.Duration; -import java.time.Instant; import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; import java.util.List; -import java.util.Map; import java.util.Optional; import java.util.Set; import java.util.concurrent.CompletableFuture; @@ -42,37 +33,26 @@ import java.util.logging.Logger; import static org.hamcrest.core.Is.is; import static org.hamcrest.core.IsCollectionContaining.hasItem; -import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertThat; import static org.junit.Assert.fail; -import static org.mockito.ArgumentMatchers.any; -import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; -@RunWith(MockitoJUnitRunner.class) public class ZookeeperStatusServiceTest { private TestingServer testingServer; private ZookeeperStatusService zookeeperStatusService; private Curator curator; - private final Timer timer = mock(Timer.class); - private final Metric metric = mock(Metric.class); private final OrchestratorContext context = mock(OrchestratorContext.class); - @Captor - private ArgumentCaptor<Map<String, String>> captor; - @Before public void setUp() throws Exception { Logger.getLogger("").setLevel(LogLevel.WARNING); testingServer = new TestingServer(); curator = createConnectedCurator(testingServer); - zookeeperStatusService = new ZookeeperStatusService(curator, metric, timer); + zookeeperStatusService = new ZookeeperStatusService(curator); when(context.getTimeLeft()).thenReturn(Duration.ofSeconds(10)); when(context.isProbe()).thenReturn(false); - when(timer.currentTime()).thenReturn(Instant.ofEpochMilli(1)); } private static Curator createConnectedCurator(TestingServer server) throws InterruptedException { @@ -101,11 +81,6 @@ public class ZookeeperStatusServiceTest { @Test public void setting_host_state_is_idempotent() { - when(timer.currentTime()).thenReturn( - Instant.ofEpochMilli((1)), - Instant.ofEpochMilli((3)), - Instant.ofEpochMilli(6)); - try (MutableStatusRegistry statusRegistry = zookeeperStatusService .lockApplicationInstance_forCurrentThreadOnly(context, TestIds.APPLICATION_INSTANCE_REFERENCE)) { @@ -121,26 +96,11 @@ public class ZookeeperStatusServiceTest { } } } - - // Time - // 1 Start before lock - // 3 After acquire => orchestrator.lock.acquire-latency = 3ms - 1ms - // 6 After release => orchestrator.lock.hold-latency = 6ms - 3ms - verify(metric).set(eq("orchestrator.lock.acquire-latency"), eq(0.002), any()); - verify(metric).set(eq("orchestrator.lock.acquired"), eq(1), any()); - verify(metric).set(eq("orchestrator.lock.hold-latency"), eq(0.003), any()); - verify(metric).createContext(captor.capture()); - - assertEquals( - Map.of("app", "test-application.test-instance-key", - "tenantName", "test-tenant", - "applicationId", "test-tenant.test-application.test-instance-key"), - captor.getValue()); } @Test public void locks_are_exclusive() throws Exception { - ZookeeperStatusService zookeeperStatusService2 = new ZookeeperStatusService(curator, mock(Metric.class), new TestTimer()); + ZookeeperStatusService zookeeperStatusService2 = new ZookeeperStatusService(curator); final CompletableFuture<Void> lockedSuccessfullyFuture; try (MutableStatusRegistry statusRegistry = zookeeperStatusService @@ -165,7 +125,7 @@ public class ZookeeperStatusServiceTest { @Test public void failing_to_get_lock_closes_SessionFailRetryLoop() throws Exception { - ZookeeperStatusService zookeeperStatusService2 = new ZookeeperStatusService(curator, mock(Metric.class), new TestTimer()); + ZookeeperStatusService zookeeperStatusService2 = new ZookeeperStatusService(curator); try (MutableStatusRegistry statusRegistry = zookeeperStatusService .lockApplicationInstance_forCurrentThreadOnly(context, TestIds.APPLICATION_INSTANCE_REFERENCE)) { |