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.

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

Report conflict when trying to disable `_ttl` #7316

Closed
wants to merge 4 commits into from

Conversation

Projects
None yet
4 participants
@brwe
Copy link
Contributor

brwe commented Aug 18, 2014

_ttl could never be disabled once it was enabled.
But when trying to, no conflict was reported.

relates to #777 and #7293

brwe added some commits Aug 15, 2014

Docs: _ttl ignores all parameters except for enabled and default.
Also, add a line about that it can never be disabled after it was enabled.
_ttl: Report conflict when trying to disable _ttl
_ttl could never be disabled once it was enabled.
But when trying to, no conflict was reported.

relates to #777 and #7293

@brwe brwe added v1.4.0 labels Aug 18, 2014

@@ -238,12 +239,14 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws
@Override
public void merge(Mapper mergeWith, MergeContext mergeContext) throws MergeMappingException {
TTLFieldMapper ttlMergeWith = (TTLFieldMapper) mergeWith;
if (!mergeContext.mergeFlags().simulate()) {

This comment has been minimized.

Copy link
@jpountz

jpountz Aug 18, 2014

Contributor

I think you should not ignore it completely? The contract is that is simulate is true then conflicts are reported but changes are not performed.

This comment has been minimized.

Copy link
@brwe

brwe Aug 18, 2014

Author Contributor

Yes! Thank god for code reviews...

This comment has been minimized.

Copy link
@brwe

brwe Aug 19, 2014

Author Contributor

I added a commit and tests for this that were missing before

this.enabledState = ttlMergeWith.enabledState;
} else {
mergeContext.addConflict("_ttl cannot be disabled once it was enabled.");
}

This comment has been minimized.

Copy link
@jpountz

jpountz Aug 18, 2014

Contributor

I think this will report a conflict if _ttl was not explicitely enabled or disabled and then you try to disable it explicitely? Which is not expected?

This comment has been minimized.

Copy link
@brwe

brwe Aug 19, 2014

Author Contributor

yes, added a commit to fix this

@jpountz jpountz removed the review label Aug 18, 2014

@brwe

This comment has been minimized.

Copy link
Contributor Author

brwe commented Aug 19, 2014

Adressed all comments.

@jpountz

This comment has been minimized.

Copy link
Contributor

jpountz commented Aug 20, 2014

LGTM

@jpountz jpountz removed the review label Aug 20, 2014

@brwe brwe closed this in ab9e33e Aug 21, 2014

brwe added a commit that referenced this pull request Aug 21, 2014

_ttl: Report conflict when trying to disable _ttl
_ttl could never be disabled once it was enabled.
But when trying to, no conflict was reported.

relates to #777 and #7293

closes #7316

@spinscale spinscale added the bug label Aug 26, 2014

brwe added a commit that referenced this pull request Sep 8, 2014

_ttl: Report conflict when trying to disable _ttl
_ttl could never be disabled once it was enabled.
But when trying to, no conflict was reported.

relates to #777 and #7293

closes #7316

@clintongormley clintongormley changed the title _ttl: Report conflict when trying to disable _ttl Mapping: Report conflict when trying to disable `_ttl` Sep 8, 2014

@clintongormley clintongormley changed the title Mapping: Report conflict when trying to disable `_ttl` Report conflict when trying to disable `_ttl` Jun 7, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.