Merge pull request #11306 from nik9000/default_detect_noop

Default detect_noop to true

core/src/main/java/org/elasticsearch/action/update/UpdateRequest.java

@@ -78,7 +78,7 @@ public class UpdateRequest extends InstanceShardOperationRequest<UpdateRequest>
 
     private boolean scriptedUpsert = false;
     private boolean docAsUpsert = false;
-    private boolean detectNoop = false;
+    private boolean detectNoop = true;
 
     @Nullable
     private IndexRequest doc;
@@ -243,7 +243,7 @@ public class UpdateRequest extends InstanceShardOperationRequest<UpdateRequest>
      * The script to execute. Note, make sure not to send different script each
      * times and instead use script params if possible with the same
      * (automatically compiled) script.
-     * 
+     *
      * @deprecated Use {@link #script(Script)} instead
      */
     @Deprecated
@@ -256,7 +256,7 @@ public class UpdateRequest extends InstanceShardOperationRequest<UpdateRequest>
      * The script to execute. Note, make sure not to send different script each
      * times and instead use script params if possible with the same
      * (automatically compiled) script.
-     * 
+     *
      * @deprecated Use {@link #script(Script)} instead
      */
     @Deprecated
@@ -267,7 +267,7 @@ public class UpdateRequest extends InstanceShardOperationRequest<UpdateRequest>
 
     /**
      * The language of the script to execute.
-     * 
+     *
      * @deprecated Use {@link #script(Script)} instead
      */
     @Deprecated
@@ -286,7 +286,7 @@ public class UpdateRequest extends InstanceShardOperationRequest<UpdateRequest>
 
     /**
      * Add a script parameter.
-     * 
+     *
      * @deprecated Use {@link #script(Script)} instead
      */
     @Deprecated
@@ -311,7 +311,7 @@ public class UpdateRequest extends InstanceShardOperationRequest<UpdateRequest>
 
     /**
      * Sets the script parameters to use with the script.
-     * 
+     *
      * @deprecated Use {@link #script(Script)} instead
      */
     @Deprecated
@@ -338,7 +338,7 @@ public class UpdateRequest extends InstanceShardOperationRequest<UpdateRequest>
      * The script to execute. Note, make sure not to send different script each
      * times and instead use script params if possible with the same
      * (automatically compiled) script.
-     * 
+     *
      * @deprecated Use {@link #script(Script)} instead
      */
     @Deprecated
@@ -360,7 +360,7 @@ public class UpdateRequest extends InstanceShardOperationRequest<UpdateRequest>
      *            The script type
      * @param scriptParams
      *            The script parameters
-     * 
+     *
      * @deprecated Use {@link #script(Script)} instead
      */
     @Deprecated
@@ -623,7 +623,7 @@ public class UpdateRequest extends InstanceShardOperationRequest<UpdateRequest>
     }
 
     /**
-     * Should this update attempt to detect if it is a noop?
+     * Should this update attempt to detect if it is a noop? Defaults to true.
      * @return this for chaining
      */
     public UpdateRequest detectNoop(boolean detectNoop) {
@@ -631,6 +631,9 @@ public class UpdateRequest extends InstanceShardOperationRequest<UpdateRequest>
         return this;
     }
 
+    /**
+     * Should this update attempt to detect if it is a noop? Defaults to true.
+     */
     public boolean detectNoop() {
         return detectNoop;
     }
@@ -699,16 +702,15 @@ public class UpdateRequest extends InstanceShardOperationRequest<UpdateRequest>
         this.docAsUpsert = shouldUpsertDoc;
         return this;
     }
-    
+
     public boolean scriptedUpsert(){
         return this.scriptedUpsert;
     }
-    
+
     public UpdateRequest scriptedUpsert(boolean scriptedUpsert) {
         this.scriptedUpsert = scriptedUpsert;
         return this;
     }
-    
 
     @Override
     public void readFrom(StreamInput in) throws IOException {

core/src/main/java/org/elasticsearch/action/update/UpdateRequestBuilder.java

@@ -308,6 +308,7 @@ public class UpdateRequestBuilder extends InstanceShardOperationRequestBuilder<U
 
     /**
      * Sets whether to perform extra effort to detect noop updates via docAsUpsert.
+     * Defautls to true.
      */
     public UpdateRequestBuilder setDetectNoop(boolean detectNoop) {
         request.detectNoop(detectNoop);
@@ -322,4 +323,14 @@ public class UpdateRequestBuilder extends InstanceShardOperationRequestBuilder<U
         request.scriptedUpsert(scriptedUpsert);
         return this;
     }
+
+    /**
+     * Set the new ttl of the document. Note that if detectNoop is true (the default)
+     * and the source of the document isn't changed then the ttl update won't take
+     * effect.
+     */
+    public UpdateRequestBuilder setTtl(Long ttl) {
+        request.doc().ttl(ttl);
+        return this;
+    }
 }

core/src/test/java/org/elasticsearch/document/BulkIT.java

@@ -195,7 +195,7 @@ public class BulkIT extends ESIntegTestCase {
         bulkResponse = client().prepareBulk()
                 .add(client().prepareUpdate("test", "type", "e1").setDoc("field", "2").setVersion(10)) // INTERNAL
                 .add(client().prepareUpdate("test", "type", "e1").setDoc("field", "3").setVersion(20).setVersionType(VersionType.FORCE))
-                .add(client().prepareUpdate("test", "type", "e1").setDoc("field", "3").setVersion(20).setVersionType(VersionType.INTERNAL)).get();
+                .add(client().prepareUpdate("test", "type", "e1").setDoc("field", "4").setVersion(20).setVersionType(VersionType.INTERNAL)).get();
 
         assertThat(bulkResponse.getItems()[0].getFailureMessage(), containsString("version conflict"));
         assertThat(((UpdateResponse) bulkResponse.getItems()[1].getResponse()).getVersion(), equalTo(20l));

core/src/test/java/org/elasticsearch/search/aggregations/bucket/ChildrenIT.java

@@ -255,9 +255,15 @@ public class ChildrenIT extends ESIntegTestCase {
             assertThat(count.getValue(), equalTo(4.));
 
             String idToUpdate = Integer.toString(randomInt(3));
+            /*
+             * The whole point of this test is to test these things with deleted
+             * docs in the index so we turn off detect_noop to make sure that
+             * the updates cause that.
+             */
             UpdateResponse updateResponse = client().prepareUpdate(indexName, "child", idToUpdate)
                     .setParent("1")
                     .setDoc("count", 1)
+                    .setDetectNoop(false)
                     .get();
             assertThat(updateResponse.getVersion(), greaterThan(1l));
             refresh();

core/src/test/java/org/elasticsearch/ttl/SimpleTTLIT.java

@@ -20,17 +20,21 @@
 package org.elasticsearch.ttl;
 
 import com.google.common.base.Predicate;
+
 import org.elasticsearch.action.admin.indices.mapping.get.GetMappingsResponse;
 import org.elasticsearch.action.admin.indices.mapping.put.PutMappingResponse;
 import org.elasticsearch.action.admin.indices.stats.IndicesStatsResponse;
 import org.elasticsearch.action.get.GetResponse;
 import org.elasticsearch.action.index.IndexResponse;
+import org.elasticsearch.action.update.UpdateRequestBuilder;
+import org.elasticsearch.action.update.UpdateResponse;
 import org.elasticsearch.common.settings.Settings;
 import org.elasticsearch.common.xcontent.XContentBuilder;
 import org.elasticsearch.common.xcontent.XContentFactory;
+import org.elasticsearch.index.get.GetField;
 import org.elasticsearch.test.ESIntegTestCase;
 import org.elasticsearch.test.ESIntegTestCase.ClusterScope;
-import org.junit.Test;
+import org.elasticsearch.test.ESIntegTestCase.Scope;
 
 import java.io.IOException;
 import java.util.Locale;
@@ -39,9 +43,17 @@ import java.util.concurrent.TimeUnit;
 
 import static org.elasticsearch.common.settings.Settings.settingsBuilder;
 import static org.elasticsearch.common.xcontent.XContentFactory.jsonBuilder;
-import static org.elasticsearch.test.ESIntegTestCase.Scope;
 import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertAcked;
-import static org.hamcrest.Matchers.*;
+import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertNoFailures;
+import static org.hamcrest.Matchers.both;
+import static org.hamcrest.Matchers.equalTo;
+import static org.hamcrest.Matchers.greaterThan;
+import static org.hamcrest.Matchers.hasKey;
+import static org.hamcrest.Matchers.is;
+import static org.hamcrest.Matchers.lessThan;
+import static org.hamcrest.Matchers.lessThanOrEqualTo;
+import static org.hamcrest.Matchers.notNullValue;
+import static org.hamcrest.Matchers.nullValue;
 
 @ClusterScope(scope= Scope.SUITE, numDataNodes = 1)
 public class SimpleTTLIT extends ESIntegTestCase {
@@ -63,7 +75,6 @@ public class SimpleTTLIT extends ESIntegTestCase {
                 .build();
     }
 
-    @Test
     public void testSimpleTTL() throws Exception {
         assertAcked(prepareCreate("test")
                 .addMapping("type1", XContentFactory.jsonBuilder()
@@ -200,7 +211,7 @@ public class SimpleTTLIT extends ESIntegTestCase {
         assertThat(getResponse.isExists(), equalTo(false));
     }
 
-    @Test // issue 5053
+    // issue 5053
     public void testThatUpdatingMappingShouldNotRemoveTTLConfiguration() throws Exception {
         String index = "foo";
         String type = "mytype";
@@ -220,6 +231,63 @@ public class SimpleTTLIT extends ESIntegTestCase {
         assertTTLMappingEnabled(index, type);
     }
 
+    /**
+     * Test that updates with detect_noop set to true (the default) that don't
+     * change the source don't change the ttl. This is unexpected behavior and
+     * documented in ttl-field.asciidoc. If this behavior changes it is safe to
+     * rewrite this test to reflect the new behavior and to change the
+     * documentation.
+     */
+    public void testNoopUpdate() throws IOException {
+        assertAcked(prepareCreate("test")
+                .addMapping("type1", XContentFactory.jsonBuilder()
+                        .startObject()
+                        .startObject("type1")
+                        .startObject("_timestamp").field("enabled", true).endObject()
+                        .startObject("_ttl").field("enabled", true).endObject()
+                        .endObject()
+                        .endObject()));
+        ensureYellow("test");
+
+        long aLongTime = 10000000;
+        long firstTtl = aLongTime * 3;
+        long secondTtl = aLongTime * 2;
+        long thirdTtl = aLongTime * 1;
+        IndexResponse indexResponse = client().prepareIndex("test", "type1", "1").setSource("field1", "value1")
+                .setTTL(firstTtl).setRefresh(true).get();
+        assertTrue(indexResponse.isCreated());
+        assertThat(getTtl("type1", 1), both(lessThan(firstTtl)).and(greaterThan(secondTtl)));
+
+        // Updating with the default detect_noop without a change to the document doesn't change the ttl.
+        UpdateRequestBuilder update = client().prepareUpdate("test", "type1", "1").setDoc("field1", "value1").setTtl(secondTtl);
+        assertThat(updateAndGetTtl(update), both(lessThan(firstTtl)).and(greaterThan(secondTtl)));
+
+        // Updating with the default detect_noop with a change to the document does change the ttl.
+        update = client().prepareUpdate("test", "type1", "1").setDoc("field1", "value2").setTtl(secondTtl);
+        assertThat(updateAndGetTtl(update), both(lessThan(secondTtl)).and(greaterThan(thirdTtl)));
+
+        // Updating with detect_noop=true without a change to the document doesn't change the ttl.
+        update = client().prepareUpdate("test", "type1", "1").setDoc("field1", "value2").setTtl(secondTtl).setDetectNoop(true);
+        assertThat(updateAndGetTtl(update), both(lessThan(secondTtl)).and(greaterThan(thirdTtl)));
+
+        // Updating with detect_noop=false without a change to the document does change the ttl.
+        update = client().prepareUpdate("test", "type1", "1").setDoc("field1", "value2").setTtl(thirdTtl).setDetectNoop(false);
+        assertThat(updateAndGetTtl(update), lessThan(thirdTtl));
+    }
+
+    private long updateAndGetTtl(UpdateRequestBuilder update) {
+        UpdateResponse updateResponse = update.setFields("_ttl").get();
+        assertThat(updateResponse.getShardInfo().getFailed(), equalTo(0));
+        // You can't actually fetch _ttl from an update so we use a get.
+        return getTtl(updateResponse.getType(), updateResponse.getId());
+    }
+
+    private long getTtl(String type, Object id) {
+        GetResponse getResponse = client().prepareGet("test", type, id.toString()).setFields("_ttl").setRealtime(true).execute()
+                .actionGet();
+        return ((Number) getResponse.getField("_ttl").getValue()).longValue();
+    }
+
     private void assertTTLMappingEnabled(String index, String type) throws IOException {
         String errMsg = String.format(Locale.ROOT, "Expected ttl field mapping to be enabled for %s/%s", index, type);
 

core/src/test/java/org/elasticsearch/update/UpdateNoopIT.java

@@ -19,6 +19,7 @@
 
 package org.elasticsearch.update;
 
+import org.elasticsearch.action.update.UpdateRequestBuilder;
 import org.elasticsearch.action.update.UpdateResponse;
 import org.elasticsearch.common.xcontent.XContentBuilder;
 import org.elasticsearch.common.xcontent.XContentFactory;
@@ -44,8 +45,10 @@ public class UpdateNoopIT extends ESIntegTestCase {
         updateAndCheckSource(4, fields("bar", null));
         updateAndCheckSource(4, fields("bar", null));
         updateAndCheckSource(5, fields("bar", "foo"));
+        // detect_noop defaults to true
+        updateAndCheckSource(5, null, fields("bar", "foo"));
 
-        assertEquals(3, totalNoopUpdates());
+        assertEquals(4, totalNoopUpdates());
     }
 
     @Test
@@ -210,7 +213,8 @@ public class UpdateNoopIT extends ESIntegTestCase {
     }
 
     /**
-     * Totally empty requests are noop if and only if detect noops is true.
+     * Totally empty requests are noop if and only if detect noops is true and
+     * its true by default.
      */
     @Test
     public void totallyEmpty() throws Exception {
@@ -223,6 +227,7 @@ public class UpdateNoopIT extends ESIntegTestCase {
                 .endObject());
         update(true, 1, XContentFactory.jsonBuilder().startObject().endObject());
         update(false, 2, XContentFactory.jsonBuilder().startObject().endObject());
+        update(null, 2, XContentFactory.jsonBuilder().startObject().endObject());
     }
 
     private XContentBuilder fields(Object... fields) throws IOException {
@@ -237,17 +242,23 @@ public class UpdateNoopIT extends ESIntegTestCase {
     }
 
     private void updateAndCheckSource(long expectedVersion, XContentBuilder xContentBuilder) {
-        UpdateResponse updateResponse = update(true, expectedVersion, xContentBuilder);
+        updateAndCheckSource(expectedVersion, true, xContentBuilder);
+    }
+
+    private void updateAndCheckSource(long expectedVersion, Boolean detectNoop, XContentBuilder xContentBuilder) {
+        UpdateResponse updateResponse = update(detectNoop, expectedVersion, xContentBuilder);
         assertEquals(updateResponse.getGetResult().sourceRef().toUtf8(), xContentBuilder.bytes().toUtf8());
     }
 
-    private UpdateResponse update(boolean detectNoop, long expectedVersion, XContentBuilder xContentBuilder) {
-        UpdateResponse updateResponse = client().prepareUpdate("test", "type1", "1")
+    private UpdateResponse update(Boolean detectNoop, long expectedVersion, XContentBuilder xContentBuilder) {
+        UpdateRequestBuilder updateRequest = client().prepareUpdate("test", "type1", "1")
                 .setDoc(xContentBuilder.bytes().toUtf8())
                 .setDocAsUpsert(true)
-                .setDetectNoop(detectNoop)
-                .setFields("_source")
-                .execute().actionGet();
+                .setFields("_source");
+        if (detectNoop != null) {
+            updateRequest.setDetectNoop(detectNoop);
+        }
+        UpdateResponse updateResponse = updateRequest.get();
         assertThat(updateResponse.getGetResult(), notNullValue());
         assertEquals(expectedVersion, updateResponse.getVersion());
         return updateResponse;

docs/reference/docs/update.asciidoc

@@ -114,25 +114,23 @@ If both `doc` and `script` is specified, then `doc` is ignored. Best is
 to put your field pairs of the partial document in the script itself.
 
 [float]
-=== Detecting noop
-
-By default if `doc` is specified then the document is always updated even
-if the merging process doesn't cause any changes.  Specifying `detect_noop`
-as `true` will cause Elasticsearch to check if there are changes and, if
-there aren't, turn the update request into a noop. For example:
-
+=== Detecting noop updates
+If `doc` is specified its value is merged with the existing `_source`. By
+default the document is only reindexed if the new `_source` field differs from
+the old. Setting `detect_noop` to `false` will cause Elasticsearch to always
+update the document even if it hasn't changed. For example:
 [source,js]
 --------------------------------------------------
 curl -XPOST 'localhost:9200/test/type1/1/_update' -d '{
     "doc" : {
         "name" : "new_name"
     },
-    "detect_noop": true
+    "detect_noop": false
 }'
 --------------------------------------------------
 
-If `name` was `new_name` before the request was sent then the entire update
-request is ignored.
+If `name` was `new_name` before the request was sent then document is still
+reindexed.
 
 [[upserts]]
 [float]

docs/reference/mapping/fields/ttl-field.asciidoc

@@ -104,3 +104,7 @@ may still be retrieved before they are purged.
 How many deletions are handled by a single <<docs-bulk,`bulk`>> request. The
 default value is `10000`.
 
+==== Note on `detect_noop`
+If an update tries to update just the `_ttl` without changing the `_source` of
+the document it's expiration time won't be updated if `detect_noop` is `true`.
+In 2.1 `detect_noop` defaults to `true`.

docs/reference/migration/migrate_2_1.asciidoc

@@ -24,4 +24,14 @@ GET /my_index/_search?scroll=2m
 
 Scroll requests sorted by `_doc` have been optimized to more efficiently resume
 from where the previous request stopped, so this will have the same performance
-characteristics as the former `scan` search type.
+characteristics as the former `scan` search type.
+
+=== Update changes
+
+==== Updates now `detect_noop` by default
+
+We've switched the default value of the `detect_noop` option from `false` to
+`true`. This means that Elasticsearch will ignore updates that don't change
+source unless you explicitly set `"detect_noop": false`. `detect_noop` was
+always computationally cheap compared to the expense of the update which can be
+thought of as a delete operation followed by an index operation.

rest-api-spec/src/main/resources/rest-api-spec/test/update/75_ttl.yaml

@@ -56,7 +56,7 @@
  - lte:   { _ttl: 100000}
  - gt:    { _ttl: 10000}
 
-# duration
+# seconds
 
  - do:
       update:
@@ -66,6 +66,7 @@
           body:
             doc:        { foo: baz }
             upsert:     { foo: bar }
+            detect_noop:  false
           ttl:       20s
 
  - do:
@@ -89,4 +90,3 @@
           body:      { foo: bar }
           ttl:       20s
           timestamp: 2013-06-23T18:14:40
-