Add config option to require indices have an _ID field

Author: aprudhomme

docs/server_configuration.rst

@@ -117,6 +117,11 @@ Example server configuration
      - If enabled, the server will use a separate executor for commit operations instead of using the indexing executor.
      - false
 
+   * - requireIdField
+     - bool
+     - If enabled, all indices must contain an _ID field to be started.
+     - false
+
 .. list-table:: `Threadpool Configuration <https://github.com/Yelp/nrtsearch/blob/master/src/main/java/com/yelp/nrtsearch/server/config/ThreadPoolConfiguration.java>`_ (``threadPoolConfiguration.*``)
    :widths: 25 10 50 25
    :header-rows: 1

src/main/java/com/yelp/nrtsearch/server/config/NrtsearchConfig.java

@@ -113,6 +113,7 @@ public class NrtsearchConfig {
   private final boolean verifyReplicationIndexId;
   private final boolean useKeepAliveForReplication;
   private final DirectoryFactory.MMapGrouping mmapGrouping;
+  private final boolean requireIdField;
 
   @Inject
   public NrtsearchConfig(InputStream yamlStream) {
@@ -192,6 +193,7 @@ public NrtsearchConfig(InputStream yamlStream) {
             o -> DirectoryFactory.parseMMapGrouping(o.toString()),
             DirectoryFactory.MMapGrouping.SEGMENT);
     useSeparateCommitExecutor = configReader.getBoolean("useSeparateCommitExecutor", false);
+    requireIdField = configReader.getBoolean("requireIdField", false);
 
     List<String> indicesWithOverrides = configReader.getKeysOrEmpty("indexLiveSettingsOverrides");
     Map<String, IndexLiveSettings> liveSettingsMap = new HashMap<>();
@@ -389,6 +391,10 @@ public boolean getUseSeparateCommitExecutor() {
     return useSeparateCommitExecutor;
   }
 
+  public boolean getRequireIdField() {
+    return requireIdField;
+  }
+
   /**
    * Substitute all sub strings of the form ${FOO} with the environment variable value env[FOO].
    * Variable names may only contain letters, numbers, and underscores. If a variable is not present

src/main/java/com/yelp/nrtsearch/server/index/StartIndexProcessor.java

@@ -42,6 +42,7 @@ public class StartIndexProcessor {
   private final IndexStateManager indexStateManager;
   private final boolean remoteCommit;
   private final int discoveryFileUpdateIntervalMs;
+  private final boolean requireIdField;
   private static final Logger logger = LoggerFactory.getLogger(StartIndexProcessor.class);
 
   /**
@@ -53,20 +54,23 @@ public class StartIndexProcessor {
    * @param indexStateManager index state manager
    * @param remoteCommit whether to commit to remote state
    * @param discoveryFileUpdateIntervalMs interval to update backends from discovery file
+   * @param requireIdField whether the index must have an _ID field defined
    */
   public StartIndexProcessor(
       String serviceName,
       String ephemeralId,
       RemoteBackend remoteBackend,
       IndexStateManager indexStateManager,
       boolean remoteCommit,
-      int discoveryFileUpdateIntervalMs) {
+      int discoveryFileUpdateIntervalMs,
+      boolean requireIdField) {
     this.serviceName = serviceName;
     this.ephemeralId = ephemeralId;
     this.remoteBackend = remoteBackend;
     this.indexStateManager = indexStateManager;
     this.remoteCommit = remoteCommit;
     this.discoveryFileUpdateIntervalMs = discoveryFileUpdateIntervalMs;
+    this.requireIdField = requireIdField;
   }
 
   public StartIndexResponse process(IndexState indexState, StartIndexRequest startIndexRequest)
@@ -96,6 +100,13 @@ public StartIndexResponse process(IndexState indexState, StartIndexRequest start
   private StartIndexResponse processInternal(
       IndexState indexState, StartIndexRequest startIndexRequest)
       throws StartIndexProcessorException {
+
+    if (requireIdField && indexState.getIdFieldDef().isEmpty()) {
+      throw new IllegalArgumentException(
+          String.format(
+              "Index %s must have an _ID field defined to be started", indexState.getName()));
+    }
+
     final ShardState shardState = indexState.getShard(0);
     final Mode mode = startIndexRequest.getMode();
     final long primaryGen;

src/main/java/com/yelp/nrtsearch/server/state/BackendGlobalState.java

@@ -450,7 +450,8 @@ private StartIndexResponse startIndex(
                 .getIndexStartConfig()
                 .getDataLocationType()
                 .equals(IndexDataLocationType.REMOTE),
-            getConfiguration().getDiscoveryFileUpdateIntervalMs());
+            getConfiguration().getDiscoveryFileUpdateIntervalMs(),
+            getConfiguration().getRequireIdField());
     try {
       return startIndexHandler.process(indexStateManager.getCurrent(), startIndexRequest);
     } catch (StartIndexProcessorException e) {

src/test/java/com/yelp/nrtsearch/server/config/NrtsearchConfigTest.java

@@ -232,4 +232,18 @@ public void testCustomMaxClauseCount() {
     NrtsearchConfig luceneConfig = getForConfig(config);
     assertEquals(2048, luceneConfig.getMaxClauseCount());
   }
+
+  @Test
+  public void testRequireIdField_default() {
+    String config = "nodeName: \"server_foo\"";
+    NrtsearchConfig luceneConfig = getForConfig(config);
+    assertFalse(luceneConfig.getRequireIdField());
+  }
+
+  @Test
+  public void testRequireIdField_set() {
+    String config = "requireIdField: true";
+    NrtsearchConfig luceneConfig = getForConfig(config);
+    assertTrue(luceneConfig.getRequireIdField());
+  }
 }

src/test/java/com/yelp/nrtsearch/server/grpc/IndexStartTest.java

@@ -29,6 +29,7 @@
 import java.io.IOException;
 import java.util.Collections;
 import java.util.HashSet;
+import java.util.List;
 import org.apache.lucene.replicator.nrt.ReplicaDeleterManager;
 import org.junit.After;
 import org.junit.Rule;
@@ -888,4 +889,41 @@ public void testCommitIndexNotStarted() throws IOException {
       assertTrue(e.getMessage().contains("index \"test_index:0\" was not started"));
     }
   }
+
+  @Test
+  public void testNoIdWhenNotRequired() throws IOException {
+    TestServer server =
+        TestServer.builder(folder)
+            .withAutoStartConfig(true, Mode.PRIMARY, 0, IndexDataLocationType.LOCAL)
+            .withAdditionalConfig("requireIdField: false")
+            .build();
+    server.createIndex("test_index");
+    server.registerFields(
+        "test_index",
+        List.of(Field.newBuilder().setName("field1").setType(FieldType.ATOM).build()));
+    assertFalse(server.isStarted("test_index"));
+    server.startIndexV2(StartIndexV2Request.newBuilder().setIndexName("test_index").build());
+    assertTrue(server.isStarted("test_index"));
+  }
+
+  @Test
+  public void testNoIdWhenRequired() throws IOException {
+    TestServer server =
+        TestServer.builder(folder)
+            .withAutoStartConfig(true, Mode.PRIMARY, 0, IndexDataLocationType.LOCAL)
+            .withAdditionalConfig("requireIdField: true")
+            .build();
+    server.createIndex("test_index");
+    server.registerFields(
+        "test_index",
+        List.of(Field.newBuilder().setName("field1").setType(FieldType.ATOM).build()));
+    assertFalse(server.isStarted("test_index"));
+    try {
+      server.startIndexV2(StartIndexV2Request.newBuilder().setIndexName("test_index").build());
+      fail();
+    } catch (StatusRuntimeException e) {
+      assertTrue(
+          e.getMessage().contains("Index test_index must have an _ID field defined to be started"));
+    }
+  }
 }