Merge some of update by document and script #629
Conversation
|
I'm not in love with the Updatable name but it gets the job done. |
|
This cuts out quite a bit of code. Nice. I'm also not a big fan of the name Updatable (sounds also like an interface). Should we call it Source (as you do in the code) or Object? Object is probably reserved... How does elasticsearch treat a script? Same as a document? Means isn't a script extending a document? Are all the functions you put in Updatable supported by Script? |
|
Everything on the Updatable object should be respected by updates, yeah. Source isn't really right because it really only works as the command you can give to an update action. UpdateCommand? Its a bit lame, but it makes some sense. |
|
Can Updatable be used alone? Probably not, so we could make it an abstract object to make it clear, it should not be used directly. UpdateAbstract? UpdatableAbstract? I don't want to block this pull request because of a name discussion. If we don't come to a name conclusion fast, lets just pick one :-) Can you make the changes.txt update a little bit more extensive? |
|
On Tue, Jun 10, 2014 at 5:00 PM, Nicolas Ruflin notifications@github.com
Sure, will do in the morning. |
|
(+1) for AbstractUpdateAction. Lets go with this one. Can you update this also in the code? |
|
Yup. Will do in the morning. On Tue, Jun 10, 2014 at 5:20 PM, Nicolas Ruflin notifications@github.com
|
|
Updated. Travis seems upset with me at the moment though. |
|
I restarted the build manually. Lets see what the outcome will be :-) |
|
@nik9000 Can you merge in master? Not sure if that is what makes Travis unhappy ... |
|
Rebased on top of master. I can squash if you think that'll help. |
|
It starts building again :-) Not sure what the merge conflict will be... So if you can do the merge for me, more then welcome :-) |
|
Squashed all my commits to one. Since its on top of master it should just merge clean. |
|
The build doesn't look too good :-( |
|
BTW: I just updated the build to es 1.2.1 |
|
I fixed an obvious error and the tests look fixed locally but travis is still unhappy. |
|
I would assume that this "test failure" is based on your change: But you said, this is working locally? |
|
I can get it to fail every third time I run it locally. I'll see if I can track it down. |
|
Strange. Can you try optimize instead of refresh? Perhaps something changed to in the delete API? |
|
Now we need to merge in the other changes. |
|
Rebased and pushed. I'm running the tests again locally because I'll probably beat travis. It'd be useful to generate changes.txt on release rather then when as part of the pull request - it seems like the most likely thing to fail merges. |
These share many of the same options so it makes sense to merge them. This also adds support for things like _retry_on_conflict and _routing to updates by scripts. Closes ruflin#628 Closes ruflin#627
|
Builds were looking good but I had to amend to fix a merge error I left in changes.txt. |
|
Ok. Will run out of battery in a minute. Will merge it later. |
Merge some of update by document and script
|
Merged. Thx a lot for all the extra effort. |
|
Thanks! On Fri, Jun 13, 2014 at 4:56 PM, Nicolas Ruflin notifications@github.com
|
If the document doesn't change then don't push a new copy of it into the index. Elasticsearch doesn't do this by default and updates are very expensive. This should prevent any that aren't really required. I have an instinct that says this is a pretty significant fraction of updates at WMF. Instinct only, though. We graph it in ganglia so we can be sure if it is worth it. Requires ruflin/Elastica#629 or we'll lose retry on conflict support. Change-Id: I533e08ac15d5e48d1de8b36e3b86a419f339be38
These share many of the same options so it makes sense to merge them.
Closes #628
Closes #627