Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Remove deprecated created and found from index, delete and bulk #25516

Merged
merged 3 commits into from
Jul 7, 2017
Merged

Remove deprecated created and found from index, delete and bulk #25516

merged 3 commits into from
Jul 7, 2017

Conversation

olcbean
Copy link
Contributor

@olcbean olcbean commented Jul 3, 2017

The created and found fields in index and delete responses became obsolete after the introduction of the result field in index, update and delete responses (#19566).

After deprecating the created and found fields in 5.x (#19633), now they are removed.

Fixes #19630

@elasticmachine
Copy link
Collaborator

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

nik9000
nik9000 requested changes Jul 3, 2017
View reviewed changes
Copy link
Member

@nik9000 nik9000 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left two minor things. Otherwise I think this looks good!

Thanks for working on it @olcbean!

core/src/main/java/org/elasticsearch/action/delete/DeleteResponse.java
@Override
public DeleteResponse build() {
DeleteResponse deleteResponse = new DeleteResponse(shardId, type, id, seqNo, primaryTerm, version, found);
DeleteResponse deleteResponse = new DeleteResponse(shardId, type, id, seqNo, primaryTerm, version,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you should pass the Result here. Maybe also add an assertion in the ctor that the result is either Result.DELETED or Result.NOT_FOUND.

core/src/main/java/org/elasticsearch/action/index/IndexResponse.java
@Override
public IndexResponse build() {
IndexResponse indexResponse = new IndexResponse(shardId, type, id, seqNo, primaryTerm, version, created);
IndexResponse indexResponse = new IndexResponse(shardId, type, id, seqNo, primaryTerm, version,
result == Result.CREATED ? true : false);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same deal here as on Delete. I think you should pass the result into the ctor and assert that it is sane.

docs/reference/docs/bulk.asciidoc
@@ -138,7 +136,6 @@ The result of this bulk operation is:
"successful": 1,
"failed": 0
},
"created": true,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm so glad we assert that these are right now!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is exactly the kind of thing I'd miss when removing these without the assertions.

@nik9000 nik9000 self-assigned this Jul 3, 2017
@nik9000 nik9000 added :Clients/Java Low Level REST Client Minimal dependencies Java Client for Elasticsearch >breaking v6.0.0 >enhancement labels Jul 3, 2017
@nik9000
Copy link
Member

nik9000 commented Jul 3, 2017

Marked as breaking because it is but that is ok because it removes deprecated response parameters that have had an alternate way to read them for most of the 5.x series.

@olcbean
Copy link
Contributor Author

olcbean commented Jul 5, 2017

@nik9000 thanks for the feedback! Just pushed a new commit

@nik9000
Copy link
Member

nik9000 commented Jul 7, 2017

Ah! I see now. The way you had it first was better. I'll revert the second commit and merge.

@nik9000 nik9000 merged commit 2ba9fd2 into elastic:master Jul 7, 2017
@nik9000
Copy link
Member

nik9000 commented Jul 7, 2017

Thanks @olcbean! I've merged to master.

jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Jul 7, 2017
@jasontedor
Merge branch 'master' into disk-threshold-settings-validation
c98b772 
* master:
  Remove deprecated created and found from index, delete and bulk (elastic#25516)
  fix testEnsureVersionCompatibility for 5.5.0 release
  fix Version.v6_0_0 min compatibility version to 5.5.0
  Add bwc indices for 5.5.0
  Add v5_5_1 constant
  [DOCS] revise high level client Search Scroll API docs (elastic#25599)
  Improve REST error handling when endpoint does not support HTTP verb, add OPTIONS support (elastic#24437)
  Avoid SecurityException in repository-S3 on DefaultS3OutputStream.flush() (elastic#25254)
  [Tests] Add tests for CompletionSuggestionBuilder#build() (elastic#25575)
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Jul 7, 2017
@jasontedor
Merge branch 'master' into restore-local-checkpoint-tracker
4290e2d 
* master: (42 commits)
  Harden global checkpoint tracker
  Remove deprecated created and found from index, delete and bulk (elastic#25516)
  fix testEnsureVersionCompatibility for 5.5.0 release
  fix Version.v6_0_0 min compatibility version to 5.5.0
  Add bwc indices for 5.5.0
  Add v5_5_1 constant
  [DOCS] revise high level client Search Scroll API docs (elastic#25599)
  Improve REST error handling when endpoint does not support HTTP verb, add OPTIONS support (elastic#24437)
  Avoid SecurityException in repository-S3 on DefaultS3OutputStream.flush() (elastic#25254)
  [Tests] Add tests for CompletionSuggestionBuilder#build() (elastic#25575)
  Enable cross-setting validation
  [Docs] Fix typo in bootstrap-checks.asciidoc (elastic#25597)
  Index ids in binary form. (elastic#25352)
  bwc checkout should fetch from all remotes
  IndexingIT should check for global checkpoints regardless of master version
  [Tests] Add tests for PhraseSuggestionBuilder#build() (elastic#25571)
  Remove unused class MinimalMap (elastic#25590)
  [Docs] Document Scroll API for Java High Level REST Client (elastic#25554)
  Disable date field mapping changing (elastic#25285)
  Allow BWC Testing against a specific branch (elastic#25510)
  ...
@olcbean
Copy link
Contributor Author

olcbean commented Jul 8, 2017

@nik9000 Excellent, this is what I was hoping the final change to look like. Sorry about the confusion with the belated push earlier :)

@olcbean olcbean deleted the remove_created branch July 8, 2017 10:54
@andrewkroh andrewkroh mentioned this pull request Jul 12, 2017
Closed
andrewkroh added a commit to andrewkroh/beats that referenced this pull request Jul 13, 2017
@andrewkroh
Added 'result' field to Elasticsearch QueryResult
e06f90f 
Added 'result' field to Elasticsearch QueryResult struct for compatibility with 6.x Index and Delete API responses.
See elastic/elasticsearch#25516 for more info.

Fixes elastic#4661
@andrewkroh andrewkroh mentioned this pull request Jul 13, 2017
Merged
ruflin pushed a commit to elastic/beats that referenced this pull request Jul 14, 2017
@andrewkroh @ruflin
Added 'result' field to Elasticsearch QueryResult
c96512d 
Added 'result' field to Elasticsearch QueryResult struct for compatibility with 6.x Index and Delete API responses.
See elastic/elasticsearch#25516 for more info.

Fixes #4661
@p365labs p365labs mentioned this pull request Aug 21, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nik9000 nik9000 nik9000 requested changes

Assignees

@nik9000 nik9000

Labels
>breaking :Clients/Java Low Level REST Client Minimal dependencies Java Client for Elasticsearch >enhancement v6.0.0-beta1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@olcbean @elasticmachine @nik9000 @colings86