Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

In 2.0 detect_noop for updates should be on by default #11282

Closed
nik9000 opened this issue May 21, 2015 · 7 comments · Fixed by #11306
Closed

In 2.0 detect_noop for updates should be on by default #11282

nik9000 opened this issue May 21, 2015 · 7 comments · Fixed by #11306
Labels
>breaking :Distributed/CRUD A catch all label for issues around indexing, updating and getting a doc by id. Not search.

Comments

@nik9000
Copy link
Member

nik9000 commented May 21, 2015

We added detect_noop to updates a while back but left it defaulting to false because there are use cases where you'd want it to be false. Things like where you want timestamps to be updated even if the document didn't change. Anyway, while 2.0 is making breaking changes is probably the time to switch from defaulting to false to true. Its worth switching because performing noop updates are expensive.
@nik9000 nik9000 mentioned this issue May 21, 2015
Closed
@clintongormley clintongormley added >breaking review :Distributed/CRUD A catch all label for issues around indexing, updating and getting a doc by id. Not search. and removed review labels May 21, 2015
@jpountz
Copy link
Contributor

jpountz commented May 21, 2015

It's not clear to me yet how common it is to run updates that don't change anything, but this no-op detection looks cheap to me, so +1 if it can help certain use-cases, I don't think it would hurt anyone.

@nik9000
Copy link
Member Author

nik9000 commented May 21, 2015

It's not clear to me yet how common it is to run updates that don't change anything, but this no-op detection looks cheap to me, so +1 if it can help certain use-cases, I don't think it would hurt anyone.

Yeah - I don't know how common it is either. If it weren't cheap I wouldn't recommend it but it seems cheap and I think folks expect noops to be cheap.

@boysmovie
Copy link

+1 it's logical for noop to be defaulted true. There are many use cases where messages get repeated with just an updated timestamp. Helps with es performance too.

@jpountz
Copy link
Contributor

jpountz commented May 22, 2015

@aanuprab I'm confused: If a timestamp field is updated then noop detection will not help since the document changed?

@boysmovie
Copy link

@jpountz for example we have thousands of sensor data that continually stream in. Lots of times the actually sensor readings don't change. But it's just the timestamp on the sensor message header. The data is all that we store in ES, excluding the timestamp header. So it will be a noop. Hope it helps.

@jpountz
Copy link
Contributor

jpountz commented May 22, 2015

OK, got it, the timestamp is not part of your documents.

@nik9000
Copy link
Member Author

nik9000 commented May 22, 2015

I'm putting together a patch for this. I noticed that you've been collecting the breaking changes in the migrate_2.0.asciidoc file so I'll have a note in there too.

nik9000 added a commit to nik9000/elasticsearch that referenced this issue May 22, 2015
@nik9000
Default detect_noop to true
2fc09d9 
detect_noop is pretty cheap and noop updates compartively expensive so this
feels like a sensible default.

Closes elastic#11282
@nik9000 nik9000 mentioned this issue May 22, 2015
Merged
@nik9000 nik9000 mentioned this issue Aug 18, 2015
Closed
nik9000 added a commit to nik9000/elasticsearch that referenced this issue Aug 27, 2015
@nik9000
Default detect_noop to true
9eb684d 
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
>breaking :Distributed/CRUD A catch all label for issues around indexing, updating and getting a doc by id. Not search.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Default detect_noop to true nik9000/elasticsearch
4 participants
@clintongormley @nik9000 @jpountz @boysmovie