Move isNewService check to TimelockAgent

changelog/@unreleased/pr-5555.v2.yml

@@ -0,0 +1,5 @@
+type: improvement
+improvement:
+  description: Move isNewService check to TimelockAgent
+  links:
+  - https://github.com/palantir/atlasdb/pull/5555

timelock-agent/src/main/java/com/palantir/timelock/config/PaxosInstallConfiguration.java

@@ -114,4 +114,10 @@ default void checkSqliteAndFileDataDirectoriesAreNotPossiblyShared() {
     default boolean doDataDirectoriesExist() {
         return dataDirectory().isDirectory();
     }
+
+    static Builder builder() {
+        return new Builder();
+    }
+
+    class Builder extends ImmutablePaxosInstallConfiguration.Builder {}
 }

timelock-agent/src/main/java/com/palantir/timelock/config/TimeLockInstallConfiguration.java

@@ -66,11 +66,9 @@ default boolean isNewServiceNode() {
                 || cluster().knownNewServers().contains(cluster().localServer());
     }
 
-    @Value.Check
-    default void check() {
-        if (!paxos().ignoreNewServiceCheck()) {
-            TimeLockPersistenceInvariants.checkPersistenceConsistentWithState(
-                    isNewServiceNode(), paxos().doDataDirectoriesExist());
-        }
+    static Builder builder() {
+        return new Builder();
     }
+
+    class Builder extends ImmutableTimeLockInstallConfiguration.Builder {}
 }

timelock-agent/src/main/java/com/palantir/timelock/paxos/TimeLockAgent.java

@@ -73,6 +73,7 @@
 import com.palantir.timelock.config.DatabaseTsBoundPersisterRuntimeConfiguration;
 import com.palantir.timelock.config.PaxosTsBoundPersisterConfiguration;
 import com.palantir.timelock.config.TimeLockInstallConfiguration;
+import com.palantir.timelock.config.TimeLockPersistenceInvariants;
 import com.palantir.timelock.config.TimeLockRuntimeConfiguration;
 import com.palantir.timelock.config.TsBoundPersisterConfiguration;
 import com.palantir.timelock.corruption.detection.CorruptionHealthReport;
@@ -134,6 +135,8 @@ public static TimeLockAgent create(
             Optional<Consumer<UndertowService>> undertowRegistrar,
             OrderableSlsVersion timeLockVersion,
             ObjectMapper objectMapper) {
+        verifyIsNewServiceInvariant(install);
+
         TimeLockDialogueServiceProvider timeLockDialogueServiceProvider =
                 createTimeLockDialogueServiceProvider(metricsManager, install, userAgent);
         PaxosResourcesFactory.TimelockPaxosInstallationContext installationContext =
@@ -387,6 +390,14 @@ private void registerCorruptionHandlerWrappedService(
                 UndertowCorruptionHandlerService.of(service, corruptionComponents.timeLockCorruptionHealthCheck()));
     }
 
+    @VisibleForTesting
+    static void verifyIsNewServiceInvariant(TimeLockInstallConfiguration install) {
+        if (!install.paxos().ignoreNewServiceCheck()) {
+            TimeLockPersistenceInvariants.checkPersistenceConsistentWithState(
+                    install.isNewServiceNode(), install.paxos().doDataDirectoriesExist());
+        }
+    }
+
     static void verifySchemaVersion(PersistedSchemaVersion persistedSchemaVersion) {
         Preconditions.checkState(
                 persistedSchemaVersion.getVersion() == SCHEMA_VERSION,

timelock-agent/src/test/java/com/palantir/atlasdb/timelock/paxos/PaxosRemoteClientsTest.java

@@ -30,9 +30,7 @@
 import com.palantir.conjure.java.api.config.service.UserAgent;
 import com.palantir.sls.versions.OrderableSlsVersion;
 import com.palantir.timelock.config.ImmutableDefaultClusterConfiguration;
-import com.palantir.timelock.config.ImmutablePaxosInstallConfiguration;
 import com.palantir.timelock.config.ImmutablePaxosTsBoundPersisterConfiguration;
-import com.palantir.timelock.config.ImmutableTimeLockInstallConfiguration;
 import com.palantir.timelock.config.PaxosInstallConfiguration;
 import com.palantir.timelock.config.SqlitePaxosPersistenceConfigurations;
 import com.palantir.timelock.config.TimeLockInstallConfiguration;
@@ -55,13 +53,13 @@ public class PaxosRemoteClientsTest {
 
     @Before
     public void setUp() {
-        paxosInstallConfiguration = ImmutablePaxosInstallConfiguration.builder()
+        paxosInstallConfiguration = PaxosInstallConfiguration.builder()
                 .isNewService(false)
                 .dataDirectory(temporaryFolder.getRoot())
                 .sqlitePersistence(SqlitePaxosPersistenceConfigurations.DEFAULT)
                 .leaderMode(PaxosInstallConfiguration.PaxosLeaderMode.SINGLE_LEADER)
                 .build();
-        installConfiguration = ImmutableTimeLockInstallConfiguration.builder()
+        installConfiguration = TimeLockInstallConfiguration.builder()
                 .cluster(ImmutableDefaultClusterConfiguration.builder()
                         .localServer("a:1")
                         .cluster(

timelock-agent/src/test/java/com/palantir/timelock/config/PaxosInstallConfigurationIntegrationTest.java

@@ -19,23 +19,14 @@
 import static org.assertj.core.api.Assertions.assertThatCode;
 import static org.assertj.core.api.Assertions.assertThatThrownBy;
 
-import com.google.common.collect.ImmutableList;
-import com.palantir.conjure.java.api.config.service.PartialServiceConfiguration;
 import java.io.File;
 import java.io.IOException;
-import java.util.Optional;
 import org.junit.Before;
 import org.junit.Rule;
 import org.junit.Test;
 import org.junit.rules.TemporaryFolder;
 
 public class PaxosInstallConfigurationIntegrationTest {
-    private static final String SERVER_A = "a";
-    private static final ClusterConfiguration CLUSTER_CONFIG = ImmutableDefaultClusterConfiguration.builder()
-            .localServer(SERVER_A)
-            .cluster(PartialServiceConfiguration.of(ImmutableList.of(SERVER_A, "b", "c"), Optional.empty()))
-            .build();
-
     @Rule
     public final TemporaryFolder temporaryFolder = new TemporaryFolder();
 
@@ -130,28 +121,28 @@ private File getAndCreateRandomSubdirectory() {
 
     private static ImmutablePaxosInstallConfiguration.Builder createPartialConfiguration(
             File dataDirectory, File sqliteDataDirectory) {
-        return ImmutablePaxosInstallConfiguration.builder()
+        return PaxosInstallConfiguration.builder()
                 .dataDirectory(dataDirectory)
                 .sqlitePersistence(ImmutableSqlitePaxosPersistenceConfiguration.builder()
                         .dataDirectory(sqliteDataDirectory)
                         .build());
     }
 
     private static void assertIsNotNewService(ImmutablePaxosInstallConfiguration.Builder partialConfiguration) {
-        assertThatCode(() -> attemptConstructTopLevelConfigWithoutOverrides(
+        assertThatCode(() -> checkPersistenceInvariants(
                         partialConfiguration.isNewService(false).build()))
                 .doesNotThrowAnyException();
-        assertThatThrownBy(() -> attemptConstructTopLevelConfigWithoutOverrides(
+        assertThatThrownBy(() -> checkPersistenceInvariants(
                         partialConfiguration.isNewService(true).build()))
                 .isInstanceOf(IllegalArgumentException.class)
                 .hasMessageContaining("This timelock server has been configured as a new stack");
     }
 
     private void assertIsNewService(ImmutablePaxosInstallConfiguration.Builder partialConfiguration) {
-        assertThatCode(() -> attemptConstructTopLevelConfigWithoutOverrides(
+        assertThatCode(() -> checkPersistenceInvariants(
                         partialConfiguration.isNewService(true).build()))
                 .doesNotThrowAnyException();
-        assertThatThrownBy(() -> attemptConstructTopLevelConfigWithoutOverrides(
+        assertThatThrownBy(() -> checkPersistenceInvariants(
                         partialConfiguration.isNewService(false).build()))
                 .isInstanceOf(IllegalArgumentException.class)
                 .hasMessageContaining("The timelock data directories do not appear to exist.");
@@ -166,11 +157,8 @@ private static File getAndCreateSubdirectory(File base, String subdirectoryName)
     }
 
     @SuppressWarnings("CheckReturnValue")
-    private static void attemptConstructTopLevelConfigWithoutOverrides(
-            PaxosInstallConfiguration paxosInstallConfiguration) {
-        ImmutableTimeLockInstallConfiguration.builder()
-                .paxos(paxosInstallConfiguration)
-                .cluster(CLUSTER_CONFIG)
-                .build();
+    private static void checkPersistenceInvariants(PaxosInstallConfiguration paxosInstallConfiguration) {
+        TimeLockPersistenceInvariants.checkPersistenceConsistentWithState(
+                paxosInstallConfiguration.isNewService(), paxosInstallConfiguration.doDataDirectoriesExist());
     }
 }

timelock-agent/src/test/java/com/palantir/timelock/config/TimeLockInstallConfigurationTest.java

@@ -17,12 +17,9 @@
 package com.palantir.timelock.config;
 
 import static org.assertj.core.api.Assertions.assertThat;
-import static org.assertj.core.api.Assertions.assertThatCode;
-import static org.assertj.core.api.Assertions.assertThatThrownBy;
 
 import com.google.common.collect.ImmutableList;
 import com.palantir.conjure.java.api.config.service.PartialServiceConfiguration;
-import com.palantir.logsafe.exceptions.SafeIllegalArgumentException;
 import com.palantir.timelock.config.PaxosInstallConfiguration.PaxosLeaderMode;
 import java.io.File;
 import java.io.IOException;
@@ -51,9 +48,6 @@ public class TimeLockInstallConfigurationTest {
     private File extantPaxosLogDirectory;
     private File extantSqliteLogDirectory;
 
-    private PaxosInstallConfiguration newService;
-    private PaxosInstallConfiguration extantService;
-
     @Before
     public void setUp() throws IOException {
         newPaxosLogDirectory = Paths.get(temporaryFolder.getRoot().toString(), "part-time-parliament")
@@ -67,7 +61,7 @@ public void setUp() throws IOException {
 
     @Test
     public void newServiceIfNewServiceFlagSetToTrue() {
-        assertThat(ImmutableTimeLockInstallConfiguration.builder()
+        assertThat(TimeLockInstallConfiguration.builder()
                         .cluster(CLUSTER_CONFIG)
                         .paxos(createPaxosInstall(true, false))
                         .build()
@@ -77,7 +71,7 @@ public void newServiceIfNewServiceFlagSetToTrue() {
 
     @Test
     public void existingServiceIfNewServiceFlagSetToFalse() {
-        assertThat(ImmutableTimeLockInstallConfiguration.builder()
+        assertThat(TimeLockInstallConfiguration.builder()
                         .cluster(CLUSTER_CONFIG)
                         .paxos(createPaxosInstall(false, true))
                         .build()
@@ -87,7 +81,7 @@ public void existingServiceIfNewServiceFlagSetToFalse() {
 
     @Test
     public void newNodeInExistingServiceRecognisedAsNew() {
-        assertThat(ImmutableTimeLockInstallConfiguration.builder()
+        assertThat(TimeLockInstallConfiguration.builder()
                         .cluster(ImmutableDefaultClusterConfiguration.builder()
                                 .localServer(SERVER_A)
                                 .cluster(PartialServiceConfiguration.of(
@@ -101,49 +95,13 @@ public void newNodeInExistingServiceRecognisedAsNew() {
                 .isTrue();
     }
 
-    @Test
-    public void newServiceNotSetNoDataDirectoryThrows() {
-        assertThatThrownBy(() -> ImmutableTimeLockInstallConfiguration.builder()
-                        .cluster(CLUSTER_CONFIG)
-                        .paxos(createPaxosInstall(false, false))
-                        .build())
-                .isInstanceOf(SafeIllegalArgumentException.class);
-    }
-
-    @Test
-    public void newServiceSetNoDataDirectoryExistsThrows() {
-        assertThatThrownBy(() -> ImmutableTimeLockInstallConfiguration.builder()
-                        .cluster(CLUSTER_CONFIG)
-                        .paxos(createPaxosInstall(true, true))
-                        .build())
-                .isInstanceOf(SafeIllegalArgumentException.class);
-    }
-
-    @Test
-    public void newServiceNotSetNoDataDirectoryDoesNotThrowWhenIgnoreFlagSet() {
-        assertThatCode(() -> ImmutableTimeLockInstallConfiguration.builder()
-                        .cluster(CLUSTER_CONFIG)
-                        .paxos(createPaxosInstall(false, false, true))
-                        .build())
-                .doesNotThrowAnyException();
-    }
-
-    @Test
-    public void newServiceSetNoDataDirectoryExistsDoesNotThrowWhenIgnoreFlagSet() {
-        assertThatCode(() -> ImmutableTimeLockInstallConfiguration.builder()
-                        .cluster(CLUSTER_CONFIG)
-                        .paxos(createPaxosInstall(true, true, true))
-                        .build())
-                .doesNotThrowAnyException();
-    }
-
     private PaxosInstallConfiguration createPaxosInstall(boolean isNewService, boolean shouldDirectoriesExist) {
         return createPaxosInstall(isNewService, shouldDirectoriesExist, false);
     }
 
     private PaxosInstallConfiguration createPaxosInstall(
             boolean isNewService, boolean shouldDirectoriesExist, boolean ignoreCheck) {
-        return ImmutablePaxosInstallConfiguration.builder()
+        return PaxosInstallConfiguration.builder()
                 .dataDirectory(shouldDirectoriesExist ? extantPaxosLogDirectory : newPaxosLogDirectory)
                 .sqlitePersistence(ImmutableSqlitePaxosPersistenceConfiguration.builder()
                         .dataDirectory(shouldDirectoriesExist ? extantSqliteLogDirectory : extantPaxosLogDirectory)

timelock-agent/src/test/java/com/palantir/timelock/config/TimeLockPersistenceInvariantsTest.java

@@ -15,8 +15,6 @@
  */
 package com.palantir.timelock.config;
 
-import static org.assertj.core.api.Assertions.assertThatCode;
-import static org.assertj.core.api.Assertions.assertThatThrownBy;
 import static org.mockito.Mockito.atLeastOnce;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.times;
@@ -42,9 +40,8 @@ public class TimeLockPersistenceInvariantsTest {
     public void doesNotCreateDirectoryForPaxosDirectoryIfNewService() throws IOException {
         File mockFile = getMockFileWith(false, true);
 
-        assertCanBuildConfiguration(ImmutablePaxosInstallConfiguration.builder()
-                .dataDirectory(mockFile)
-                .isNewService(true));
+        assertCanBuildConfiguration(
+                PaxosInstallConfiguration.builder().dataDirectory(mockFile).isNewService(true));
 
         verify(mockFile, times(0)).mkdirs();
     }
@@ -53,69 +50,12 @@ public void doesNotCreateDirectoryForPaxosDirectoryIfNewService() throws IOExcep
     public void canUseExistingDirectoryAsPaxosDirectory() throws IOException {
         File mockFile = getMockFileWith(true, false);
 
-        assertCanBuildConfiguration(ImmutablePaxosInstallConfiguration.builder()
-                .dataDirectory(mockFile)
-                .isNewService(false));
+        assertCanBuildConfiguration(
+                PaxosInstallConfiguration.builder().dataDirectory(mockFile).isNewService(false));
 
         verify(mockFile, atLeastOnce()).isDirectory();
     }
 
-    @Test
-    public void throwsIfConfiguredToBeNewServiceWithExistingDirectory() throws IOException {
-        File mockFile = getMockFileWith(true, true);
-
-        assertFailsToBuildConfiguration(ImmutablePaxosInstallConfiguration.builder()
-                .dataDirectory(mockFile)
-                .isNewService(true));
-    }
-
-    @Test
-    public void throwsIfConfiguredToBeExistingServiceWithoutDirectory() throws IOException {
-        File mockFile = getMockFileWith(false, true);
-
-        assertFailsToBuildConfiguration(ImmutablePaxosInstallConfiguration.builder()
-                .dataDirectory(mockFile)
-                .isNewService(false));
-    }
-
-    @Test
-    public void newServiceByClusterBootstrapConfigurationFailsIfDirectoryExists() throws IOException {
-        File mockFile = getMockFileWith(true, true);
-
-        ClusterConfiguration differentClusterConfig = ImmutableDefaultClusterConfiguration.builder()
-                .localServer(SERVER_A)
-                .addKnownNewServers(SERVER_A)
-                .cluster(PartialServiceConfiguration.of(ImmutableList.of(SERVER_A, "b", "c"), Optional.empty()))
-                .build();
-
-        assertThatThrownBy(ImmutableTimeLockInstallConfiguration.builder()
-                        .cluster(differentClusterConfig)
-                        .paxos(ImmutablePaxosInstallConfiguration.builder()
-                                .dataDirectory(mockFile)
-                                .isNewService(false)
-                                .build())::build)
-                .isInstanceOf(IllegalArgumentException.class);
-    }
-
-    @Test
-    public void newServiceByClusterBootstrapConfigurationSucceedsIfDirectoryDoesNotExist() throws IOException {
-        File mockFile = getMockFileWith(false, true);
-
-        ClusterConfiguration differentClusterConfig = ImmutableDefaultClusterConfiguration.builder()
-                .localServer(SERVER_A)
-                .addKnownNewServers(SERVER_A)
-                .cluster(PartialServiceConfiguration.of(ImmutableList.of(SERVER_A, "b", "c"), Optional.empty()))
-                .build();
-
-        assertThatCode(ImmutableTimeLockInstallConfiguration.builder()
-                        .cluster(differentClusterConfig)
-                        .paxos(ImmutablePaxosInstallConfiguration.builder()
-                                .dataDirectory(mockFile)
-                                .isNewService(false)
-                                .build())::build)
-                .doesNotThrowAnyException();
-    }
-
     private File getMockFileWith(boolean isDirectory, boolean canCreateDirectory) throws IOException {
         File mockFile = mock(File.class);
         when(mockFile.mkdirs()).thenReturn(canCreateDirectory);
@@ -128,17 +68,9 @@ private File getMockFileWith(boolean isDirectory, boolean canCreateDirectory) th
     @SuppressWarnings("CheckReturnValue")
     private void assertCanBuildConfiguration(ImmutablePaxosInstallConfiguration.Builder configBuilder) {
         PaxosInstallConfiguration installConfiguration = configBuilder.build();
-        ImmutableTimeLockInstallConfiguration.builder()
+        TimeLockInstallConfiguration.builder()
                 .cluster(CLUSTER_CONFIG)
                 .paxos(installConfiguration)
                 .build();
     }
-
-    private void assertFailsToBuildConfiguration(ImmutablePaxosInstallConfiguration.Builder configBuilder) {
-        PaxosInstallConfiguration installConfiguration = configBuilder.build();
-        assertThatThrownBy(ImmutableTimeLockInstallConfiguration.builder()
-                        .cluster(CLUSTER_CONFIG)
-                        .paxos(installConfiguration)::build)
-                .isInstanceOf(IllegalArgumentException.class);
-    }
 }