Skip namespace check for InMemoryKVS (#2541)

* Skip namespace check for InMemoryKVS We don't care about the namespace for InMemoryKVS, so we want to allow any namespace to be set. Previously, each product that used InMemoryKVS for their tests needed to configure the namespace to be exactly "test". * InMemoryKvs: Disallow empty namespace and empty TimeLock client
palantir · Oct 26, 2017 · 950c77f · 950c77f
1 parent ef127a6
commit 950c77f
Show file tree

Hide file tree

Showing 6 changed files with 75 additions and 13 deletions.
diff --git a/atlasdb-config/src/main/java/com/palantir/atlasdb/config/AtlasDbConfig.java b/atlasdb-config/src/main/java/com/palantir/atlasdb/config/AtlasDbConfig.java
@@ -26,6 +26,7 @@
 import com.fasterxml.jackson.databind.annotation.JsonSerialize;
 import com.google.common.base.Preconditions;
 import com.palantir.atlasdb.AtlasDbConstants;
+import com.palantir.atlasdb.memory.InMemoryAtlasDbConfig;
 import com.palantir.atlasdb.spi.KeyValueServiceConfig;
 
 @JsonDeserialize(as = ImmutableAtlasDbConfig.class)
@@ -307,7 +308,7 @@ private void checkNamespaceConfig() {
                     Preconditions.checkState(client.equals(namespace),
                             "If present, the TimeLock client config should be the same as the"
                                     + " atlas root-level namespace config.")));
-        } else {
+        } else if (!(keyValueService() instanceof InMemoryAtlasDbConfig)) {
             Preconditions.checkState(keyValueService().namespace().isPresent(),
                     "Either the atlas root-level namespace"
                             + " or the keyspace/dbName/sid config needs to be set.");
@@ -330,6 +331,11 @@ private void checkNamespaceConfig() {
                                 + " Please contact AtlasDB support to remediate this. Specific steps are required;"
                                 + " DO NOT ATTEMPT TO FIX THIS YOURSELF.");
             }
+        } else if (timelock().isPresent()) {
+            // Special case - empty timelock and empty namespace/keyspace does not make sense
+            boolean timelockClientNonEmpty = !timelock().get().client().orElse("").isEmpty();
+            Preconditions.checkState(timelockClientNonEmpty,
+                    "For InMemoryKVS, the TimeLock client should not be empty");
         }
     }
 

diff --git a/...db-config/src/test/java/com/palantir/atlasdb/config/AtlasDbConfigDeserializationTest.java b/...db-config/src/test/java/com/palantir/atlasdb/config/AtlasDbConfigDeserializationTest.java
@@ -37,7 +37,7 @@ public class AtlasDbConfigDeserializationTest {
     @Test
     public void canDeserializeAtlasDbConfig() throws IOException {
         AtlasDbConfig config = AtlasDbConfigs.load(TEST_CONFIG_FILE);
-        assertThat(config.namespace().get()).isEqualTo("test");
+        assertThat(config.namespace().get()).isEqualTo("brian");
         assertThat(config.keyValueService()).isEqualTo(new InMemoryAtlasDbConfig());
 
         assertThat(config.timelock().isPresent()).isTrue();
@@ -60,7 +60,7 @@ public void canDeserializeMinimalAtlasDbConfig() throws IOException {
     }
 
     private void assertTimeLockConfigDeserializedCorrectly(TimeLockClientConfig timeLockClientConfig) {
-        assertThat(timeLockClientConfig.getClientOrThrow()).isEqualTo("test");
+        assertThat(timeLockClientConfig.getClientOrThrow()).isEqualTo("brian");
         assertThat(timeLockClientConfig.serversList().servers()).containsExactlyInAnyOrder(
                 "timelock1:8080", "timelock2:8080", "timelock3:8080");
         assertThat(timeLockClientConfig.serversList().sslConfiguration().isPresent()).isTrue();

diff --git a/atlasdb-config/src/test/java/com/palantir/atlasdb/config/AtlasDbConfigTest.java b/atlasdb-config/src/test/java/com/palantir/atlasdb/config/AtlasDbConfigTest.java
@@ -20,6 +20,7 @@
 import static org.hamcrest.Matchers.is;
 import static org.hamcrest.Matchers.not;
 import static org.hamcrest.Matchers.nullValue;
+import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertThat;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.when;
@@ -29,6 +30,7 @@
 import org.junit.BeforeClass;
 import org.junit.Test;
 
+import com.palantir.atlasdb.memory.InMemoryAtlasDbConfig;
 import com.palantir.atlasdb.spi.KeyValueServiceConfig;
 import com.palantir.remoting.api.config.ssl.SslConfiguration;
 
@@ -216,6 +218,55 @@ public void namespaceAcceptsEmptyKvsNamespaceAndTimelockClient() {
                 .build();
     }
 
+    @Test
+    public void inMemoryConfigCanHaveEmptyNamespace() {
+        InMemoryAtlasDbConfig kvsConfig = new InMemoryAtlasDbConfig();
+        assertFalse("This test assumes the InMemoryAtlasDbConfig has no namespace by default",
+                kvsConfig.namespace().isPresent());
+        ImmutableAtlasDbConfig.builder()
+                .namespace(Optional.empty())
+                .keyValueService(kvsConfig)
+                .build();
+    }
+
+    @Test
+    public void inMemoryConfigWorksWithNonTestNamespace() {
+        InMemoryAtlasDbConfig kvsConfig = new InMemoryAtlasDbConfig();
+        assertFalse("This test assumes the InMemoryAtlasDbConfig has no namespace by default",
+                kvsConfig.namespace().isPresent());
+        ImmutableAtlasDbConfig.builder()
+                .namespace("clive")
+                .keyValueService(kvsConfig)
+                .build();
+    }
+
+    @Test
+    public void inMemoryConfigCannotHaveEmptyNamespaceWithEmptyTimelockClient() {
+        InMemoryAtlasDbConfig kvsConfig = new InMemoryAtlasDbConfig();
+        assertFalse("This test assumes the InMemoryAtlasDbConfig has no namespace by default",
+                kvsConfig.namespace().isPresent());
+        assertThatThrownBy(() -> ImmutableAtlasDbConfig.builder()
+                .namespace(Optional.empty())
+                .keyValueService(kvsConfig)
+                .timelock(TIMELOCK_CONFIG_WITH_EMPTY_CLIENT)
+                .build())
+                .isInstanceOf(IllegalStateException.class)
+                .satisfies((exception) ->
+                        assertThat(exception.getMessage(), containsString("TimeLock client should not be empty")));
+    }
+
+    // Documenting that for in-memory, we don't care what the timelock client is - it just has to be non-empty.
+    @Test
+    public void inMemoryKeyspaceAndTimelockClientCanBeDifferent() {
+        InMemoryAtlasDbConfig kvsConfig = new InMemoryAtlasDbConfig();
+        assertFalse("This test assumes the InMemoryAtlasDbConfig has no namespace by default",
+                kvsConfig.namespace().isPresent());
+        ImmutableAtlasDbConfig.builder()
+                .keyValueService(kvsConfig)
+                .timelock(TIMELOCK_CONFIG_WITH_OTHER_CLIENT)
+                .build();
+    }
+
     @Test
     public void namespaceAndTimelockClientShouldMatch() {
         assertThatThrownBy(() -> ImmutableAtlasDbConfig.builder()

diff --git a/atlasdb-config/src/test/resources/test-config.yml b/atlasdb-config/src/test/resources/test-config.yml
@@ -1,11 +1,11 @@
 atlasdb:
-  namespace: test
+  namespace: brian
 
   keyValueService:
     type: memory
 
   timelock:
-    client: test
+    client: brian
     serversList:
       servers:
         - timelock1:8080

diff --git a/atlasdb-impl-shared/src/main/java/com/palantir/atlasdb/memory/InMemoryAtlasDbConfig.java b/atlasdb-impl-shared/src/main/java/com/palantir/atlasdb/memory/InMemoryAtlasDbConfig.java
@@ -53,6 +53,6 @@ public int hashCode() {
     @Override
     @JsonIgnore
     public Optional<String> namespace() {
-        return Optional.of("test");
+        return Optional.empty();
     }
 }
diff --git a/docs/source/release_notes/release-notes.rst b/docs/source/release_notes/release-notes.rst
@@ -93,6 +93,11 @@ develop
            Note that the ``clock.monitor-exception`` metric is still incremented on every call, even if we do not log.
            (`Pull Request <https://github.com/palantir/atlasdb/pull/2456>`__)
 
+    *   - |fixed|
+        - ``InMemoryAtlasDbConfig`` now has an empty namespace, instead of "test".
+          This means that internal products will no longer have to set their root-level namespace to "test" in order to use ``InMemoryKeyValueService`` for testing.
+          (`Pull Request <https://github.com/palantir/atlasdb/pull/2541>`__)
+
     *   - |improved| |userbreak|
         - The ``ProfilingKeyValueService`` now reports its multipart log lines as a single line.
           This should improve log readability in log ingestion tools when AtlasDB is run in multithreaded environments.
@@ -205,13 +210,13 @@ v0.61.0
 =======
 v0.60.1
 =======
- 
+
 16 October 2017
 
 .. list-table::
     :widths: 5 40
     :header-rows: 1
-    
+
     *    - Type
          - Change
 
@@ -227,7 +232,7 @@ v0.60.1
            The default value for the config  is ``false`` in order to preserve previous behaviour.
            (`Pull Request 1 <https://github.com/palantir/atlasdb/pull/2390>`__ and
            `Pull Request 2 <https://github.com/palantir/atlasdb/pull/2476>`__)
-           
+
     *    - |new|
          - Timelock server can now be configured to persist the timestamp bound in the database, specifically in Cassandra/Postgres/Oracle.
            We recommend this to be configured only for cases where you absolutely need to persist all state in the database, for example,
@@ -245,7 +250,7 @@ v0.60.1
 
            Now the factory methods for the above classes return the interfaces. The actual implementation of such classes was moved to their corresponding \*Impl files.
            (`Pull Request <https://github.com/palantir/atlasdb/pull/2390>`__)
-           
+
     *    - |devbreak| |improved|
          - ``LockRefreshingTimelockService`` has been moved to the ``lock-api`` project under the package name ``com.palantir.lock.client``, and now implements
            ``AutoCloseable``, shutting down its internal executor service.
@@ -403,11 +408,11 @@ v0.58.0
 
     *    - |improved|
          - AtlasDB now logs slow queries CQL queries (via ``kvs-slow-log``) used for sweep
-           (`Pull Request <https://github.com/palantir/atlasdb/pull/2363>`__)     
-         
+           (`Pull Request <https://github.com/palantir/atlasdb/pull/2363>`__)
+
     *    - |devbreak| |fixed|
          - AtlasDB now depends on okhttp 3.8.1. This is expected to fix an issue where connections would constantly throw "shutdown" exceptions, which was likely due to a documented bug in okhttp 3.4.1.
-           (`Pull Request <https://github.com/palantir/atlasdb/pull/2343>`__)         
+           (`Pull Request <https://github.com/palantir/atlasdb/pull/2343>`__)
 
     *    - |devbreak| |improved|
          - Upgraded all uses of `http-remoting <https://github.com/palantir/http-remoting>`__ from remoting2 to remoting3, except for serialization of errors (preserved for backwards wire compatibility).