Optimize IngestDocMetadata isAvailable

docs/changelog/120753.yaml

@@ -0,0 +1,5 @@
+pr: 120753
+summary: Optimize `IngestDocMetadata` `isAvailable`
+area: Ingest Node
+type: enhancement
+issues: []

modules/ingest-common/src/test/java/org/elasticsearch/ingest/common/RenameProcessorTests.java

@@ -14,7 +14,6 @@
 import org.elasticsearch.ingest.RandomDocumentPicks;
 import org.elasticsearch.ingest.TestIngestDocument;
 import org.elasticsearch.ingest.TestTemplateService;
-import org.elasticsearch.script.Metadata;
 import org.elasticsearch.test.ESTestCase;
 
 import java.util.ArrayList;
@@ -140,42 +139,40 @@ public void testRenameExistingFieldNullValue() throws Exception {
 
  public void testRenameAtomicOperationSetFails() throws Exception {
  Map<String, Object> metadata = new HashMap<>();
- metadata.put("list", List.of("item"));
-
- IngestDocument ingestDocument = TestIngestDocument.ofMetadataWithValidator(
- metadata,
- Map.of("new_field", new Metadata.FieldProperty<>(Object.class, true, true, (k, v) -> {
- if (v != null) {
- throw new UnsupportedOperationException();
- }
- }), "list", new Metadata.FieldProperty<>(Object.class, true, true, null))
- );
- Processor processor = createRenameProcessor("list", "new_field", false, false);
+ metadata.put("_index", "foobar");
+
+ IngestDocument ingestDocument = TestIngestDocument.withDefaultVersion(metadata);
+ Processor processor = createRenameProcessor("_index", "_version_type", false, false);
  try {
  processor.execute(ingestDocument);
  fail("processor execute should have failed");
- } catch (UnsupportedOperationException e) {
+ } catch (IllegalArgumentException e) {
  // the set failed, the old field has not been removed
- assertThat(ingestDocument.getSourceAndMetadata().containsKey("list"), equalTo(true));
- assertThat(ingestDocument.getSourceAndMetadata().containsKey("new_field"), equalTo(false));
+ assertThat(
+ e.getMessage(),
+ equalTo(
+ "_version_type must be a null or one of [internal, external, external_gte] "
+ + "but was [foobar] with type [java.lang.String]"
+ )
+ );
+ assertThat(ingestDocument.getSourceAndMetadata().containsKey("_index"), equalTo(true));
+ assertThat(ingestDocument.getSourceAndMetadata().containsKey("_version_type"), equalTo(false));
  }
  }
 
  public void testRenameAtomicOperationRemoveFails() throws Exception {
  Map<String, Object> metadata = new HashMap<>();
- metadata.put("list", List.of("item"));
+ metadata.put("foo", "bar");
 
- IngestDocument ingestDocument = TestIngestDocument.ofMetadataWithValidator(
- metadata,
- Map.of("list", new Metadata.FieldProperty<>(Object.class, false, true, null))
- );
- Processor processor = createRenameProcessor("list", "new_field", false, false);
+ IngestDocument ingestDocument = TestIngestDocument.withDefaultVersion(metadata);
+ Processor processor = createRenameProcessor("_version", "new_field", false, false);
  try {
  processor.execute(ingestDocument);
  fail("processor execute should have failed");
  } catch (IllegalArgumentException e) {
- // the set failed, the old field has not been removed
- assertThat(ingestDocument.getSourceAndMetadata().containsKey("list"), equalTo(true));
+ // the remove failed, the old field has not been removed
+ assertThat(e.getMessage(), equalTo("_version cannot be removed"));
+ assertThat(ingestDocument.getSourceAndMetadata().containsKey("_version"), equalTo(true));
  assertThat(ingestDocument.getSourceAndMetadata().containsKey("new_field"), equalTo(false));
  }
  }

server/src/main/java/org/elasticsearch/ingest/IngestCtxMap.java

@@ -29,7 +29,7 @@
  *
  * The map is expected to be used by processors, server code should the typed getter and setters where possible.
  */
-class IngestCtxMap extends CtxMap<IngestDocMetadata> {
+final class IngestCtxMap extends CtxMap<IngestDocMetadata> {
 
  /**
  * Create an IngestCtxMap with the given metadata, source and default validators

server/src/main/java/org/elasticsearch/ingest/IngestDocMetadata.java

@@ -19,7 +19,7 @@
 import java.util.Map;
 import java.util.stream.Collectors;
 
-class IngestDocMetadata extends Metadata {
+final class IngestDocMetadata extends Metadata {
 
  static final Map<String, FieldProperty<?>> PROPERTIES = Map.of(
  INDEX,
@@ -42,18 +42,25 @@ class IngestDocMetadata extends Metadata {
  new FieldProperty<>(Map.class).withWritable().withNullable()
  );
 
+ private static final char UNDERSCORE = '_';
+ static {
+ // there's an optimization here in the overridden isAvailable below, but it only works if the first character of each of these
+ // keys starts with an underscore, since we know all the keys up front, though, we can just make sure that's always true
+ for (String key : PROPERTIES.keySet()) {
+ if (key.charAt(0) != UNDERSCORE) {
+ throw new IllegalArgumentException("IngestDocMetadata keys must begin with an underscore, but found [" + key + "]");
+ }
+ }
+ }
+
  protected final ZonedDateTime timestamp;
 
  IngestDocMetadata(String index, String id, long version, String routing, VersionType versionType, ZonedDateTime timestamp) {
  this(metadataMap(index, id, version, routing, versionType), timestamp);
  }
 
  IngestDocMetadata(Map<String, Object> metadata, ZonedDateTime timestamp) {
- this(metadata, PROPERTIES, timestamp);
- }
-
- IngestDocMetadata(Map<String, Object> metadata, Map<String, FieldProperty<?>> properties, ZonedDateTime timestamp) {
- super(metadata, properties);
+ super(metadata, PROPERTIES);
  this.timestamp = timestamp;
  }
 
@@ -100,4 +107,16 @@ private static void versionTypeValidator(String key, String value) {
  + "]"
  );
  }
+
+ @Override
+ public boolean isAvailable(String key) {
+ // the key cannot be null or empty because of the nature of the calling code, and this is already validated in IngestDocument
+ assert key != null && key.isEmpty() == false;
+ // we can avoid a map lookup on most keys since we know that the only keys that are 'metadata keys' for an ingest document
+ // must be keys that start with an underscore
+ if (key.charAt(0) != UNDERSCORE) {
+ return false;
+ }
+ return super.isAvailable(key);
+ }
 }

server/src/main/java/org/elasticsearch/ingest/IngestService.java

@@ -1204,8 +1204,9 @@ private static void updateIndexRequestMetadata(final IndexRequest request, final
  request.id(metadata.getId());
  request.routing(metadata.getRouting());
  request.version(metadata.getVersion());
- if (metadata.getVersionType() != null) {
- request.versionType(VersionType.fromString(metadata.getVersionType()));
+ String versionType;
+ if ((versionType = metadata.getVersionType()) != null) {
+ request.versionType(VersionType.fromString(versionType));
  }
  Number number;
  if ((number = metadata.getIfSeqNo()) != null) {

server/src/main/java/org/elasticsearch/script/CtxMap.java

@@ -14,6 +14,7 @@
 import java.util.AbstractCollection;
 import java.util.AbstractMap;
 import java.util.AbstractSet;
+import java.util.ArrayList;
 import java.util.Collection;
 import java.util.HashSet;
 import java.util.Iterator;
@@ -150,10 +151,12 @@ public Object remove(Object key) {
  @Override
  public void clear() {
  // AbstractMap uses entrySet().clear(), it should be quicker to run through the validators, then call the wrapped maps clear
- for (String key : metadata.keySet()) {
+ for (String key : new ArrayList<>(metadata.keySet())) { // copy the key set to get around the ConcurrentModificationException
  metadata.remove(key);
  }
- // TODO: this is just bogus, there isn't any case where metadata won't trip a failure above?
+ // note: this is actually bogus in the general case, though! for this to work there must be some Metadata or subclass of Metadata
+ // for which all the FieldPoperty properties of the metadata are nullable and therefore could have been removed in the previous
+ // loop -- does such a class even exist? (that is, is there any *real* CtxMap for which the previous loop didn't throw?)
  source.clear();
  }
 

server/src/test/java/org/elasticsearch/ingest/IngestCtxMapTests.java

@@ -110,7 +110,7 @@ public void testRemoveSource() {
  source.put("abc", 123);
  source.put("def", 456);
  source.put("hij", 789);
- map = new IngestCtxMap(source, new TestIngestCtxMetadata(new HashMap<>(), new HashMap<>()));
+ map = new IngestCtxMap(source, new IngestDocMetadata(new HashMap<>(Map.of("_version", 1L)), null));
 
  // Make sure there isn't a ConcurrentModificationException when removing a key from the iterator
  String removedKey = null;
@@ -129,31 +129,18 @@ public void testRemoveSource() {
  }
 
  public void testRemove() {
- String cannotRemove = "cannotRemove";
- String canRemove = "canRemove";
- Map<String, Object> metadata = new HashMap<>();
- metadata.put(cannotRemove, "value");
- map = new IngestCtxMap(
- new HashMap<>(),
- new TestIngestCtxMetadata(
- metadata,
- Map.of(
- cannotRemove,
- new Metadata.FieldProperty<>(String.class, false, true, null),
- canRemove,
- new Metadata.FieldProperty<>(String.class, true, true, null)
- )
- )
- );
- String msg = "cannotRemove cannot be removed";
+ String cannotRemove = "_version"; // writable, but not *nullable*
+ String canRemove = "_id"; // writable, and *nullable*
+ map = new IngestCtxMap(new HashMap<>(), new IngestDocMetadata(new HashMap<>(Map.of(cannotRemove, 1L)), null));
+ String msg = "_version cannot be removed";
  IllegalArgumentException err = expectThrows(IllegalArgumentException.class, () -> map.remove(cannotRemove));
  assertEquals(msg, err.getMessage());
 
  err = expectThrows(IllegalArgumentException.class, () -> map.put(cannotRemove, null));
- assertEquals("cannotRemove cannot be null", err.getMessage());
+ assertEquals("_version cannot be null", err.getMessage());
 
  err = expectThrows(IllegalArgumentException.class, () -> map.entrySet().iterator().next().setValue(null));
- assertEquals("cannotRemove cannot be null", err.getMessage());
+ assertEquals("_version cannot be null", err.getMessage());
 
  err = expectThrows(IllegalArgumentException.class, () -> {
  Iterator<Map.Entry<String, Object>> it = map.entrySet().iterator();
@@ -176,6 +163,10 @@ public void testRemove() {
  err = expectThrows(IllegalArgumentException.class, () -> map.clear());
  assertEquals(msg, err.getMessage());
 
+ // depending on iteration order, this may have been removed, so put it back before checking the size
+ map.put(canRemove, "value");
+ assertEquals("value", map.get(canRemove));
+
  assertEquals(2, map.size());
 
  map.entrySet().remove(new TestEntry(canRemove, "value"));
@@ -205,7 +196,7 @@ public void testEntryAndIterator() {
  source.put("foo", "bar");
  source.put("baz", "qux");
  source.put("noz", "zon");
- map = new IngestCtxMap(source, TestIngestCtxMetadata.withNullableVersion(metadata));
+ map = new IngestCtxMap(source, new IngestDocMetadata(metadata, null));
  md = map.getMetadata();
 
  for (Map.Entry<String, Object> entry : map.entrySet()) {
@@ -240,8 +231,10 @@ public void testEntryAndIterator() {
  assertTrue(map.containsKey("noz"));
  assertEquals(3, map.entrySet().size());
  assertEquals(3, map.size());
- map.clear();
- assertEquals(0, map.size());
+
+ // since an IngestCtxMap must have a _version (and the _version cannot be null), we can't just .clear()
+ map.entrySet().removeIf(e -> e.getKey().equals("_version") == false);
+ assertEquals(1, map.size());
  }
 
  public void testContainsValue() {

test/framework/src/main/java/org/elasticsearch/ingest/TestIngestDocument.java

@@ -10,10 +10,8 @@
 package org.elasticsearch.ingest;
 
 import org.elasticsearch.common.lucene.uid.Versions;
-import org.elasticsearch.common.util.Maps;
 import org.elasticsearch.core.Tuple;
 import org.elasticsearch.index.VersionType;
-import org.elasticsearch.script.Metadata;
 import org.elasticsearch.test.ESTestCase;
 
 import java.util.HashMap;
@@ -24,53 +22,30 @@
  */
 public class TestIngestDocument {
  public static final long DEFAULT_VERSION = 12345L;
- private static String VERSION = IngestDocument.Metadata.VERSION.getFieldName();
-
- /**
- * Create an IngestDocument for testing that pass an empty mutable map for ingestMetaata
- */
- public static IngestDocument withNullableVersion(Map<String, Object> sourceAndMetadata) {
- return ofIngestWithNullableVersion(sourceAndMetadata, new HashMap<>());
- }
+ private static final String VERSION = IngestDocument.Metadata.VERSION.getFieldName();
 
  /**
  * Create an {@link IngestDocument} from the given sourceAndMetadata and ingestMetadata and a version validator that allows null
  * _versions. Normally null _version is not allowed, but many tests don't care about that invariant.
  */
- public static IngestDocument ofIngestWithNullableVersion(Map<String, Object> sourceAndMetadata, Map<String, Object> ingestMetadata) {
- Map<String, Object> source = new HashMap<>(sourceAndMetadata);
- Map<String, Object> metadata = Maps.newHashMapWithExpectedSize(IngestDocument.Metadata.values().length);
- for (IngestDocument.Metadata m : IngestDocument.Metadata.values()) {
- String key = m.getFieldName();
- if (sourceAndMetadata.containsKey(key)) {
- metadata.put(key, source.remove(key));
- }
- }
- return new IngestDocument(new IngestCtxMap(source, TestIngestCtxMetadata.withNullableVersion(metadata)), ingestMetadata);
- }
-
- /**
- * Create an {@link IngestDocument} with {@link #DEFAULT_VERSION} as the _version metadata, if _version is not already present.
- */
- public static IngestDocument withDefaultVersion(Map<String, Object> sourceAndMetadata) {
+ public static IngestDocument withDefaultVersion(Map<String, Object> sourceAndMetadata, Map<String, Object> ingestMetadata) {
  if (sourceAndMetadata.containsKey(VERSION) == false) {
  sourceAndMetadata = new HashMap<>(sourceAndMetadata);
  sourceAndMetadata.put(VERSION, DEFAULT_VERSION);
  }
- return new IngestDocument(sourceAndMetadata, new HashMap<>());
+ return new IngestDocument(sourceAndMetadata, ingestMetadata);
  }
 
  /**
- * Create an IngestDocument with a metadata map and validators. The metadata map is passed by reference, not copied, so callers
- * can observe changes to the map directly.
+ * Create an {@link IngestDocument} with {@link #DEFAULT_VERSION} as the _version metadata, if _version is not already present.
  */
- public static IngestDocument ofMetadataWithValidator(Map<String, Object> metadata, Map<String, Metadata.FieldProperty<?>> properties) {
- return new IngestDocument(new IngestCtxMap(new HashMap<>(), new TestIngestCtxMetadata(metadata, properties)), new HashMap<>());
+ public static IngestDocument withDefaultVersion(Map<String, Object> sourceAndMetadata) {
+ return withDefaultVersion(sourceAndMetadata, new HashMap<>());
  }
 
  /**
  * Create an empty ingest document for testing.
- *
+ * <p>
  * Adds the required {@code "_version"} metadata key with value {@link #DEFAULT_VERSION}.
  */
  public static IngestDocument emptyIngestDocument() {