Removing isCreated and isFound from the Java API #19645

a2lin · 2016-07-28T07:55:44Z

Closes #19631.
This is cleanup work from #19566, where @nik9000 suggested trying to nuke the isCreated and isFound methods. I've combined nuking the two methods with removing UpdateHelper.Operation in favor of DocWriteResponse.Operation here.

…ation enum

elasticmachine · 2016-07-28T07:55:45Z

Can one of the admins verify this patch?

clintongormley · 2016-07-28T12:06:18Z

This should be for v6 only, so we'll have to wait until we master moves to v6

nik9000 · 2016-07-28T12:11:19Z

If it is just the Java methods that is ok for 5.0 I think. I'll look in a
bit when I'm properly awake.

On Jul 28, 2016 8:06 AM, "Clinton Gormley" notifications@github.com wrote:

This should be for v6 only, so we'll have to wait until we master moves to
v6

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#19645 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AANLolx4_BPcUUMzQYfKGU0ls_9r9nbEks5qaJs-gaJpZM4JW9EI
.

nik9000 · 2016-07-28T13:09:15Z

core/src/test/java/org/elasticsearch/action/IndicesRequestIT.java

        UpdateRequest updateRequest = new UpdateRequest(indexOrAlias, "type", "id").doc("field1", "value1");
        UpdateResponse updateResponse = internalCluster().coordOnlyNodeClient().update(updateRequest).actionGet();
-        assertThat(updateResponse.isCreated(), equalTo(false));
+        assertThat(updateResponse.getOperation(), not(DocWriteResponse.Operation.CREATE));


I'd prefer assertEquals(DocWriteResponse.Operation.INDEX, updateResponse.getOperations());.

I prefer equality here because it is a stronger test. If we have the information we may as well use it. I don't like using Hamcrest for strait equality because it can get overly complicated and I don't mind switching the argument order to match normal junit.

nik9000 · 2016-07-28T13:11:22Z

@clintongormley this only removes these from the Java API and keeps the REST API. We've been fairly willing to make breaking changes like this in the 5.0 process in the past (like refresh -> refreshPolicy).

nik9000 · 2016-07-28T13:31:38Z

core/src/test/java/org/elasticsearch/action/IndicesRequestIT.java

        UpdateRequest updateRequest = new UpdateRequest(indexOrAlias, "type", "id").upsert("field", "value").doc("field1", "value1");
        UpdateResponse updateResponse = internalCluster().coordOnlyNodeClient().update(updateRequest).actionGet();
-        assertThat(updateResponse.isCreated(), equalTo(true));
+        assertThat(updateResponse.getOperation(), equalTo(DocWriteResponse.Operation.CREATE));


Personally I'd do this with assertEquals as well but you can absolutely keep this if you prefer it. We certainly don't have a standard for when hamcrest is a good idea. I expect there are folks in the Elasticsearch project who prefer hamcrest all the time so they can have consistent argument ordering. I don't, only using it for more complex assertions. But, yeah, if you like hamcrest then keep it this way.

nik9000 · 2016-07-28T13:42:33Z

@a2lin:

It looks like we're talking about whether this should be in 6.0 or 5.0. I thought 5.0 but maybe I was wrong. If it has to wait until 6.0 that means we'll leave it stalled until we branch 5.0 from master.

As far as code review, I left a few line notes:

I'd prefer not to use not for the assertions at all.
I prefer assertEquals(foo, bar) over assertThat(bar, equalTo(foo)) but not enough to make you change it everywhere. If you prefer it my way then go ahead and change it.
There are a few error messages that we

None of these are major. The middle one'd be a fair amount of work but only if you want to do it. The non-test changes all look good to me.

nik9000 · 2016-07-28T14:53:39Z

Another thing: can you rename _operation to operation? I can do it if you don't want, but I figure you might be up for it. You can do it in this PR or another, either is fine with me.

The reason we want to rename it is that _ is used for document metadata. At least that is what we're thinking right now. And _operation isn't really document metadata, it is response data.

nik9000 · 2016-07-28T15:38:40Z

Another thing: can you rename _operation to operation? I can do it if you don't want, but I figure you might be up for it. You can do it in this PR or another, either is fine with me.

The reason we want to rename it is that _ is used for document metadata. At least that is what we're thinking right now. And _operation isn't really document metadata, it is response data.

Ignore that. I'll open up another issue. I had a voice chat with a few people and I'll open up another issue.

nik9000 · 2016-07-28T16:09:10Z

Ignore that. I'll open up another issue. I had a voice chat with a few people and I'll open up another issue.

#19664

a2lin · 2016-07-28T16:13:44Z

@nik9000 is it still a good idea to change all the hamcrest matchers to assertEquals if the other parts of the test have assertThat/equalTo? I've changed the ones where assertEquals seemed to be the majority to try and be consistent. Let me know if you want the others to be changed too?

nik9000 · 2016-07-28T16:15:37Z

@nik9000 is it still a good idea to change all the hamcrest matchers to assertEquals if the other parts of the test have assertThat/equalTo?

It is up to you. I'd do it for the lines that I touched but you certainly don't have to.

nik9000 · 2016-07-28T16:18:40Z

OK. It looks good to me and I can merge it if @clintongormley is ok with it going into 5.0 and you ( @a2lin ) are done with it.

a2lin · 2016-07-28T16:27:40Z

I am done with it, but I haven't run gradle check on this yet to make sure the tests pass.

a2lin · 2016-07-28T16:28:09Z

I'll probably do that in about 8 hours as I'm out of morning-time :)

nik9000 · 2016-07-28T17:30:39Z

@elasticmachine , test this

nik9000 · 2016-07-28T18:07:23Z

Jenkins caught this:
core/src/test/java/org/elasticsearch/index/mapper/DynamicMappingIT.java:103: Line is longer than 140 characters (found 168).

Maybe there is more, not sure.

a2lin · 2016-07-29T07:55:14Z

@nik9000 I think this code should pass CI now. I've also made the assertEquals change.

nik9000 · 2016-07-29T15:57:19Z

I've pulled this back to v5.0.0 because I believe that is correct. I also replaced breaking with the new breaking-java that tells folks that this really only breaks anyone that links to the java code (transport client, plugins, etc).

I'll review again.

nik9000 · 2016-07-29T16:00:18Z

OK - looks good to me. I'll merge in a bit.

@a2lin , do you have any interest in #19664, hopefully the final change in this sequence before 6.0.

a2lin · 2016-07-29T16:04:16Z

Sure, I'll give #19664 a shot. Thanks for the reviews!

a2lin added 2 commits July 27, 2016 21:36

elastic#19631 Removing isCreated and isFound from Java Api

c0a2354

Changing UpdateHelper's Operation enum to the DocWriteResponse's Oper…

7355231

…ation enum

clintongormley added >breaking stalled :Distributed Indexing/CRUD A catch all label for issues around indexing, updating and getting a doc by id. Not search. v6.0.0-alpha1 labels Jul 28, 2016

nik9000 reviewed Jul 28, 2016
View reviewed changes

a2lin force-pushed the 19631_remove_iscreated_isfound branch from 88f6cb5 to 1c444bc Compare July 28, 2016 16:01

Removed hamcrest 'not' and cleaning up error messages

2d5a473

a2lin force-pushed the 19631_remove_iscreated_isfound branch from 1c444bc to 2d5a473 Compare July 28, 2016 16:05

a2lin added 2 commits July 28, 2016 21:15

Fixing integration tests & style

cc771c1

Converted hamcrest assertThat matchers to assertEquals

e3450c9

a2lin force-pushed the 19631_remove_iscreated_isfound branch from fb28254 to e3450c9 Compare July 29, 2016 07:19

nik9000 added v5.0.0-beta1 >breaking-java and removed stalled v6.0.0-alpha1 >breaking labels Jul 29, 2016

nik9000 merged commit 119026b into elastic:master Jul 29, 2016

clintongormley added v5.0.0-alpha5 and removed v5.0.0-beta1 labels Aug 4, 2016

Removing isCreated and isFound from the Java API #19645

Removing isCreated and isFound from the Java API #19645

Uh oh!

Conversation

a2lin commented Jul 28, 2016

Uh oh!

elasticmachine commented Jul 28, 2016

Uh oh!

clintongormley commented Jul 28, 2016

Uh oh!

nik9000 commented Jul 28, 2016

Uh oh!

nik9000 Jul 28, 2016

Choose a reason for hiding this comment

Uh oh!

nik9000 commented Jul 28, 2016

Uh oh!

nik9000 Jul 28, 2016

Choose a reason for hiding this comment

Uh oh!

nik9000 commented Jul 28, 2016

Uh oh!

nik9000 commented Jul 28, 2016

Uh oh!

nik9000 commented Jul 28, 2016

Uh oh!

nik9000 commented Jul 28, 2016

Uh oh!

a2lin commented Jul 28, 2016

Uh oh!

nik9000 commented Jul 28, 2016

Uh oh!

nik9000 commented Jul 28, 2016

Uh oh!

a2lin commented Jul 28, 2016

Uh oh!

a2lin commented Jul 28, 2016

Uh oh!

nik9000 commented Jul 28, 2016

Uh oh!

nik9000 commented Jul 28, 2016

Uh oh!

a2lin commented Jul 29, 2016

Uh oh!

nik9000 commented Jul 29, 2016

Uh oh!

nik9000 commented Jul 29, 2016

Uh oh!

a2lin commented Jul 29, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants