-
Notifications
You must be signed in to change notification settings - Fork 735
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
Apply RetryOnConflict on Bulk documents #2184
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR. Change LGTM. Could you add an entry to the changelog? And can you check in the test suite if there is an easy way to add a test for this?
Normally we make changes to 8.x first and then backport to 7.x. But currently there is quite a large PR ongoing against 8.x to make it 8 compatible: #2181 What I propose is to get this in but in parallel follow up with a PR against the 8.x branch to ensure we don't forget about it. Otherwise I worry we will miss this feature in 8.x.
@csabavirag I did just trigger the test suite and it seems to break one of the tests. It seems that in some cases the object is |
I pushed a new commit with does not set RetryOnConflict if it has already been set on the document/script. I know Output of
I also updated the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change LGTM, waiting on CI to run through.
@@ -8,6 +8,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 | |||
|
|||
### Backward Compatibility Breaks | |||
### Added | |||
* If not expicitly set, use `retry_on_conflict` from Client configuration in Bulk updates [#2184](https://github.com/ruflin/Elastica/pull/2184) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we put this under "fixed"? sounds more like a bug fix but also happy to keep it under Added.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First I added to that section. Then I moved to Added. TBH, I would leave it up to you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets keep it, I can always clean it up later.
@csabavirag Thank you for the contribution! Could you follow up with a PR against 8.x. The big 8.x change made it in and we should make sure this change also makes it into 8.x |
This is a follow up PR to #2184 and replicates changes on 8.x branch.
This PR is created to solve the issue on the missing RetryOnConflict (retry_on_conflict) metadata attribute when adding documents in bulk (detailed at #2182)