fix-update-document-options-not-set #1047

Merged
merged 1 commit into from Mar 15, 2017

Conversation

Projects
None yet
4 participants
@olchick
Contributor

olchick commented Feb 14, 2016

Comes from 1046

Improvement for updateDocument method: not set option 'retry_on_conflict' if it is empty in Client's config.

@ewgRa

This comment has been minimized.

Show comment
Hide comment
@ewgRa

ewgRa Feb 15, 2016

Collaborator

@olchick For me it seems like broke BC.

For now Elastica always sends retry_on_conflict=0 by default. With your PR it will be taken from elasticsearch configuration and it can be not 0, for such developers it will be broke BC.

More right make something like "unsetConfig('retry_on_conflict')" or "setConfig('retry_on_conflict', null)" and than client can omit it when perform request.

Also what with 'version' field from original issue? I think if Amazon care about "retry_on_conflict" it will be also raise errors for "version", and your PR solve only retry_on_conflict.

Collaborator

ewgRa commented Feb 15, 2016

@olchick For me it seems like broke BC.

For now Elastica always sends retry_on_conflict=0 by default. With your PR it will be taken from elasticsearch configuration and it can be not 0, for such developers it will be broke BC.

More right make something like "unsetConfig('retry_on_conflict')" or "setConfig('retry_on_conflict', null)" and than client can omit it when perform request.

Also what with 'version' field from original issue? I think if Amazon care about "retry_on_conflict" it will be also raise errors for "version", and your PR solve only retry_on_conflict.

@Tobion

This comment has been minimized.

Show comment
Hide comment
@Tobion

Tobion Mar 12, 2017

Collaborator

@ewgRa retry_on_conflict=0 is the default in ES. How is that possible to change that?

Collaborator

Tobion commented Mar 12, 2017

@ewgRa retry_on_conflict=0 is the default in ES. How is that possible to change that?

@ewgRa

This comment has been minimized.

Show comment
Hide comment
Collaborator

ewgRa commented Mar 12, 2017

@Tobion

This comment has been minimized.

Show comment
Hide comment
@Tobion

Tobion Mar 12, 2017

Collaborator

You said there is a configuration which can be changed and due that not sending the default value would be a BC break for those people.

With your PR it will be taken from elasticsearch configuration and it can be not 0, for such developers it will be broke BC.

So what configuration are you talking about?

Collaborator

Tobion commented Mar 12, 2017

You said there is a configuration which can be changed and due that not sending the default value would be a BC break for those people.

With your PR it will be taken from elasticsearch configuration and it can be not 0, for such developers it will be broke BC.

So what configuration are you talking about?

@ewgRa

This comment has been minimized.

Show comment
Hide comment
@ewgRa

ewgRa Mar 12, 2017

Collaborator

@Tobion you are right. By default it is "the update will fail with a version conflict exception", if it is omitted, also like "default", and all this work as "0".

So, there is will be no BC break.

Collaborator

ewgRa commented Mar 12, 2017

@Tobion you are right. By default it is "the update will fail with a version conflict exception", if it is omitted, also like "default", and all this work as "0".

So, there is will be no BC break.

@Tobion

This comment has been minimized.

Show comment
Hide comment
@Tobion

Tobion Mar 13, 2017

Collaborator

Good, then I think this is good to be merged.

Collaborator

Tobion commented Mar 13, 2017

Good, then I think this is good to be merged.

@ruflin

This comment has been minimized.

Show comment
Hide comment
@ruflin

ruflin Mar 15, 2017

Owner

@Tobion Didn't check this one in detail. If it looks good, feel free to merge. @olchick Can you potentially add an entry to the changelog?

Owner

ruflin commented Mar 15, 2017

@Tobion Didn't check this one in detail. If it looks good, feel free to merge. @olchick Can you potentially add an entry to the changelog?

@Tobion Tobion merged commit a9c69f1 into ruflin:master Mar 15, 2017

3 checks passed

Scrutinizer 1 updated code elements
Details
codecov/project 69.97% (target 60.00%)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

Tobion added a commit that referenced this pull request Mar 15, 2017

@Tobion

This comment has been minimized.

Show comment
Hide comment
@Tobion

Tobion Mar 15, 2017

Collaborator

Added changelog

Collaborator

Tobion commented Mar 15, 2017

Added changelog

mhernik pushed a commit to mhernik/Elastica that referenced this pull request Jul 24, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment