Default detect_noop to true #11306
Default detect_noop to true #11306
Conversation
clintongormley
added
>breaking
review
:Distributed/CRUD
| 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: | ||
| By default if `doc` is specified then the document only updated even if the |
There was a problem hiding this comment.
"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..."
|
Updated with @clintongormley's language. Thanks. Its much clearer. |
|
@jpountz, should I just take this one? |
|
Yes, please :) |
nik9000
force-pushed
the
default_detect_noop
branch
from
bc8a10e
to
b6ddcd0
Compare
|
This badly needed a rebase so I did it. Rerunning tests. |
|
I'd love to get this into 2.0.0 somehow - please review? |
| === Updates now `detect_noop` by default | ||
|
|
||
| We've switched the default value of the `detect_noop` option from `false` to | ||
| `true` because its computationally cheap and we think its what users expect. |
|
LGTM |
|
Ok - this looks to be approved for merge but needs conflicts resolved so I'll squash and rebase. And if the conflicts resolve easily I'll merge. |
nik9000
force-pushed
the
default_detect_noop
branch
from
cf02912
to
bc04c87
Compare
|
Ok - rebasing shows a failing Rest test. I'll work on that and ask for another review when I can. |
nik9000
force-pushed
the
default_detect_noop
branch
from
b82f480
to
5cbb2f2
Compare
|
OK - fixed the rest test and rebased so it can merge. |
|
Is it possible to make |
Yeah - I'll have a look. Also, since this isn't going to make 2.0 I'll have to move/change some documentation. |
@jpountz I went with the documentation option. Its pretty easy to just always update the document if it has a ttl set but I'm not super clear on the interplay between default ttl and updates. Nor could I figure out how to clear the ttl with an update so I couldn't test to make sure I wasn't breaking that. So I went the easy way and just documented that the expiration doesn't change if the document doesn't by default. |
nik9000
force-pushed
the
default_detect_noop
branch
from
ae6588f
to
2084027
Compare
|
Squashed and rebased onto master. Changed the migrate_2_1.asciidoc wording slightly. |
nik9000
force-pushed
the
default_detect_noop
branch
from
2084027
to
bddd84d
Compare
|
@jpountz ready for another round of review I think. |
| @@ -104,3 +104,6 @@ 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 doesn't change the `_source` document it's expiration time won't | |||
There was a problem hiding this comment.
Maybe this should be more specific eg. "If an update tries to update the _ttl value but doesn't change the _source document"?
also s/it's/its/
There was a problem hiding this comment.
It'll be done in the next push.
|
Sorry for the delay. I left two minor comments but in general it looks good to ne |
Ok - moved the tests and fixed the docs as requested. |
|
LGTM |
|
OK! I'll resolve squash and rebase. If the rebase is clean enough I'll push to master. |
nik9000
force-pushed
the
default_detect_noop
branch
from
42aecde
to
e4c4305
Compare
detect_noop is pretty cheap and noop updates compartively expensive so this feels like a sensible default. Also had to do some testing and documentation around how _ttl works with detect_noop. Closes elastic#11282
nik9000
force-pushed
the
default_detect_noop
branch
from
e4c4305
to
9eb684d
Compare
|
All tests pass! Hurray! Merging. |
detect_noop is pretty cheap and noop updates compartively expensive so this
feels like a sensible default.
Closes #11282