Adding _operation field to index, update, delete response.#19566
Adding _operation field to index, update, delete response.#19566nik9000 merged 3 commits intoelastic:masterfrom
Conversation
Conflicts: src/main/java/org/elasticsearch/rest/action/update/RestUpdateAction.java
|
Can one of the admins verify this patch? |
|
I've assigned it to myself because I plan on having a look, probably my Monday morning. If anyone else wants to review before me feel free. |
| static final String _ID = "_id"; | ||
| static final String _VERSION = "_version"; | ||
| static final String _OPERATION = "_operation"; | ||
| } |
There was a problem hiding this comment.
These Fields classes have fallen out of favor in the past six months. Rather than add a new constant I'd just add a new string below, similar to what I did when I added forced_refresh. If it doesn't bloat the PR too much you can remove the whole Fields class and replace with strings at the call sites.
|
@a2lin I left a lot of feedback, all of it minor. I'm happy to talk about any of the points I left - a few of them are "can you try making this mostly related change?" I'd personally try and start those in a separate commit and give up if the change starts to get out of hand. Those are more nice-to-haves. |
|
@elasticmachine , test this. |
|
@nik9000 Thanks so much for your detailed feedback! I've made the easier fixes to make (and fixed the broken unit test; oops). I think it's better to nuke the isFound / isCreated in a following PR, but let me know if you think I should include those commits into this one. Same thing for the UpdateHelper.Operation change. |
Sounds good to me. I reviewed it one more time and it all looks great. I'm I'll have CI run on it one more time and if it passes I'll merge it. @elasticmachine, test this again. |
|
OK! All tests passed so I've merged! This is the first time I merged only after having run tests using the robot. Exciting! Thanks for this! @a2lin! |
|
And the elasticsearch robot didn't catch a build failure. I'm not sure why yet but I'll push a fix for it. Likely it is a failure caused by something changes since you forked this. I'll dig. |
|
Yeah, looks like the robot didn't test your change on master, instead testing against you branch point. I think. I'll bring it up to the robots minders. I pushed a182e35. |
|
Maybe it was just a race. That test that it didn't catch was something I merged fairly recently. So maybe it just hadn't seen it. |
|
Oh, oops :( I should have rebased against tip of master. Thanks! |
No big deal. We all merge in different ways so we don't have a policy. And we're trying out the robot, learning about it. |
Revival of #9736
Closes #9642
Closes #19267
I attempted to follow the steps suggested by @nik9000 in #9736.
I was unsure of whether the IndexResponse#isCreated method and "created": field should be changed, as the version of ES is way past 2.0.0, when the original comment was written, so I left them.
Performing the bulk request shown in #19267 now results in the following:
{"_index":"test","_type":"test","_id":"1","_version":1,"_operation":"create","forced_refresh":false,"_shards":{"total":2,"successful":1,"failed":0},"status":201}{"_index":"test","_type":"test","_id":"1","_version":1,"_operation":"noop","forced_refresh":false,"_shards":{"total":2,"successful":1,"failed":0},"status":200}