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.

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

Default detect_noop to true #11306

Merged
merged 1 commit into from Aug 27, 2015

Conversation

Projects
None yet
3 participants
@nik9000
Contributor

nik9000 commented May 22, 2015

detect_noop is pretty cheap and noop updates compartively expensive so this
feels like a sensible default.

Closes #11282

@nik9000

This comment has been minimized.

Show comment
Hide comment
@nik9000

nik9000 May 26, 2015

Contributor

Updated with @clintongormley's language. Thanks. Its much clearer.

Contributor

nik9000 commented May 26, 2015

Updated with @clintongormley's language. Thanks. Its much clearer.

@jpountz jpountz self-assigned this Jun 19, 2015

@nik9000

This comment has been minimized.

Show comment
Hide comment
@nik9000

nik9000 Jul 29, 2015

Contributor

@jpountz, should I just take this one?

Contributor

nik9000 commented Jul 29, 2015

@jpountz, should I just take this one?

@jpountz

This comment has been minimized.

Show comment
Hide comment
@jpountz

jpountz Jul 29, 2015

Contributor

Yes, please :)

Contributor

jpountz commented Jul 29, 2015

Yes, please :)

@nik9000 nik9000 assigned nik9000 and unassigned jpountz Jul 29, 2015

@nik9000

This comment has been minimized.

Show comment
Hide comment
@nik9000

nik9000 Aug 3, 2015

Contributor

This badly needed a rebase so I did it. Rerunning tests.

Contributor

nik9000 commented Aug 3, 2015

This badly needed a rebase so I did it. Rerunning tests.

@nik9000 nik9000 added v2.0.0 and removed v2.0.0-beta1 labels Aug 11, 2015

@nik9000

This comment has been minimized.

Show comment
Hide comment
@nik9000

nik9000 Aug 11, 2015

Contributor

I'd love to get this into 2.0.0 somehow - please review?

Contributor

nik9000 commented Aug 11, 2015

I'd love to get this into 2.0.0 somehow - please review?

@jpountz

View changes

Show outdated Hide outdated docs/reference/migration/migrate_2_0.asciidoc Outdated
@jpountz

This comment has been minimized.

Show comment
Hide comment
@jpountz

jpountz Aug 11, 2015

Contributor

LGTM

Contributor

jpountz commented Aug 11, 2015

LGTM

@nik9000

This comment has been minimized.

Show comment
Hide comment
@nik9000

nik9000 Aug 12, 2015

Contributor

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.

Contributor

nik9000 commented Aug 12, 2015

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

This comment has been minimized.

Show comment
Hide comment
@nik9000

nik9000 Aug 12, 2015

Contributor

Ok - rebasing shows a failing Rest test. I'll work on that and ask for another review when I can.

Contributor

nik9000 commented Aug 12, 2015

Ok - rebasing shows a failing Rest test. I'll work on that and ask for another review when I can.

@nik9000

This comment has been minimized.

Show comment
Hide comment
@nik9000

nik9000 Aug 13, 2015

Contributor

OK - fixed the rest test and rebased so it can merge.

Contributor

nik9000 commented Aug 13, 2015

OK - fixed the rest test and rebased so it can merge.

@jpountz

This comment has been minimized.

Show comment
Hide comment
@jpountz

jpountz Aug 14, 2015

Contributor

Is it possible to make detect_noop ttl-aware? If not, or if it would make things complicated, I don't think it would be too bad but then we should document it?

Contributor

jpountz commented Aug 14, 2015

Is it possible to make detect_noop ttl-aware? If not, or if it would make things complicated, I don't think it would be too bad but then we should document it?

@nik9000

This comment has been minimized.

Show comment
Hide comment
@nik9000

nik9000 Aug 14, 2015

Contributor

Is it possible to make detect_noop ttl-aware? If not, or if it would make things complicated, I don't think it would be too bad but then we should document it?

Yeah - I'll have a look. Also, since this isn't going to make 2.0 I'll have to move/change some documentation.

Contributor

nik9000 commented Aug 14, 2015

Is it possible to make detect_noop ttl-aware? If not, or if it would make things complicated, I don't think it would be too bad but then we should document it?

Yeah - I'll have a look. Also, since this isn't going to make 2.0 I'll have to move/change some documentation.

@nik9000 nik9000 added v2.1.0 and removed v2.0.0 labels Aug 14, 2015

@nik9000

This comment has been minimized.

Show comment
Hide comment
@nik9000

nik9000 Aug 14, 2015

Contributor

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.

Contributor

nik9000 commented Aug 14, 2015

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

This comment has been minimized.

Show comment
Hide comment
@nik9000

nik9000 Aug 24, 2015

Contributor

Squashed and rebased onto master. Changed the migrate_2_1.asciidoc wording slightly.

Contributor

nik9000 commented Aug 24, 2015

Squashed and rebased onto master. Changed the migrate_2_1.asciidoc wording slightly.

@nik9000

This comment has been minimized.

Show comment
Hide comment
@nik9000

nik9000 Aug 24, 2015

Contributor

@jpountz ready for another round of review I think.

Contributor

nik9000 commented Aug 24, 2015

@jpountz ready for another round of review I think.

@jpountz

View changes

Show outdated Hide outdated docs/reference/mapping/fields/ttl-field.asciidoc Outdated
@jpountz

View changes

Show outdated Hide outdated rest-api-spec/src/main/resources/rest-api-spec/test/update/75_ttl.yaml Outdated
@jpountz

This comment has been minimized.

Show comment
Hide comment
@jpountz

jpountz Aug 25, 2015

Contributor

Sorry for the delay. I left two minor comments but in general it looks good to ne

Contributor

jpountz commented Aug 25, 2015

Sorry for the delay. I left two minor comments but in general it looks good to ne

@nik9000

This comment has been minimized.

Show comment
Hide comment
@nik9000

nik9000 Aug 26, 2015

Contributor

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.

Contributor

nik9000 commented Aug 26, 2015

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.

@jpountz

This comment has been minimized.

Show comment
Hide comment
@jpountz

jpountz Aug 26, 2015

Contributor

LGTM

Contributor

jpountz commented Aug 26, 2015

LGTM

@nik9000

This comment has been minimized.

Show comment
Hide comment
@nik9000

nik9000 Aug 27, 2015

Contributor

OK! I'll resolve squash and rebase. If the rebase is clean enough I'll push to master.

Contributor

nik9000 commented Aug 27, 2015

OK! I'll resolve squash and rebase. If the rebase is clean enough I'll push to master.

Default detect_noop to true
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 #11282
@nik9000

This comment has been minimized.

Show comment
Hide comment
@nik9000

nik9000 Aug 27, 2015

Contributor

All tests pass! Hurray! Merging.

Contributor

nik9000 commented Aug 27, 2015

All tests pass! Hurray! Merging.

nik9000 added a commit that referenced this pull request Aug 27, 2015

@nik9000 nik9000 merged commit 38fdacd into elastic:master Aug 27, 2015

1 check passed

CLA Commit author has signed the CLA
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment