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 | |
parent | 9837e9b7ff4b7541db0caede471bf418f3ded02a (diff) |
Revert "Add Orchestrator application lock metrics"
-rw-r--r-- | jdisc_core/src/main/java/com/yahoo/jdisc/core/SystemTimer.java | 7 | ||||
-rw-r--r-- | node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/time/TestTimer.java (renamed from jdisc_core/src/main/java/com/yahoo/jdisc/test/TestTimer.java) | 4 | ||||
-rw-r--r-- | node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/task/util/process/ProcessFactoryImplTest.java | 2 | ||||
-rw-r--r-- | orchestrator/src/main/java/com/yahoo/vespa/orchestrator/status/ZookeeperStatusService.java | 60 | ||||
-rw-r--r-- | orchestrator/src/test/java/com/yahoo/vespa/orchestrator/OrchestratorImplTest.java | 7 | ||||
-rw-r--r-- | orchestrator/src/test/java/com/yahoo/vespa/orchestrator/model/ModelTestUtils.java | 6 | ||||
-rw-r--r-- | orchestrator/src/test/java/com/yahoo/vespa/orchestrator/resources/HostResourceTest.java | 5 | ||||
-rw-r--r-- | orchestrator/src/test/java/com/yahoo/vespa/orchestrator/status/ZookeeperStatusServiceTest.java | 46 |
8 files changed, 15 insertions, 122 deletions
diff --git a/jdisc_core/src/main/java/com/yahoo/jdisc/core/SystemTimer.java b/jdisc_core/src/main/java/com/yahoo/jdisc/core/SystemTimer.java index 40fce9fec54..15e234079b0 100644 --- a/jdisc_core/src/main/java/com/yahoo/jdisc/core/SystemTimer.java +++ b/jdisc_core/src/main/java/com/yahoo/jdisc/core/SystemTimer.java @@ -3,8 +3,6 @@ package com.yahoo.jdisc.core; import com.yahoo.jdisc.Timer; -import java.time.Instant; - /** * A timer which returns the System time * @@ -16,9 +14,4 @@ public class SystemTimer implements Timer { public long currentTimeMillis() { return System.currentTimeMillis(); } - - @Override - public Instant currentTime() { - return Instant.now(); - } } diff --git a/jdisc_core/src/main/java/com/yahoo/jdisc/test/TestTimer.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/time/TestTimer.java index 4d274a33a1f..beadeeed4a3 100644 --- a/jdisc_core/src/main/java/com/yahoo/jdisc/test/TestTimer.java +++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/time/TestTimer.java @@ -1,13 +1,11 @@ // Copyright 2018 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. -package com.yahoo.jdisc.test; +package com.yahoo.vespa.hosted.node.admin.task.util.time; import com.yahoo.jdisc.Timer; import java.time.Duration; /** - * A {@link Timer} to be used in tests when the advancement of time needs to be controlled. - * * @author hakonhall */ public class TestTimer implements Timer { diff --git a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/task/util/process/ProcessFactoryImplTest.java b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/task/util/process/ProcessFactoryImplTest.java index 09b6e65aca5..e66b3a7aed2 100644 --- a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/task/util/process/ProcessFactoryImplTest.java +++ b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/task/util/process/ProcessFactoryImplTest.java @@ -2,7 +2,7 @@ package com.yahoo.vespa.hosted.node.admin.task.util.process; import com.yahoo.vespa.hosted.node.admin.task.util.file.UnixPath; -import com.yahoo.jdisc.test.TestTimer; +import com.yahoo.vespa.hosted.node.admin.task.util.time.TestTimer; import org.junit.Test; import org.mockito.ArgumentCaptor; 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)) { |