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.

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

Added types options to DeleteByQueryRequest #23265

Merged
merged 5 commits into from Mar 2, 2017

Conversation

Projects
None yet
7 participants
@kunal642
Copy link
Contributor

commented Feb 20, 2017

related to issue #21984

@elasticmachine

This comment has been minimized.

Copy link

commented Feb 20, 2017

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

This comment has been minimized.

Copy link
Contributor

commented Feb 21, 2017

I admit these requests in java are quite messy. Could you call getSearchRequest.types(...) instead?

@kunal642

This comment has been minimized.

Copy link
Contributor Author

commented Feb 21, 2017

@nik9000 what i get from your comment is that you do not want to add the types method and the user should use getSearchRequest.types(...) instead
am i correct?

@nik9000

This comment has been minimized.

Copy link
Contributor

commented Feb 21, 2017

@nik9000 what i get from your comment is that you do not want to add the types method and the user should use getSearchRequest.types(...) instead
am i correct?

Yes. I'm just sure it is worth adding this because that is an option.

@kunal642

This comment has been minimized.

Copy link
Contributor Author

commented Feb 21, 2017

@nik9000 then the same should be followed for indices also.
getSearchRequest.indices(...) can be used to add the indices to the request and the indices method should also be removed from DeleteByQueryRequest.

@tlrx

This comment has been minimized.

Copy link
Member

commented Feb 22, 2017

getSearchRequest.indices(...) can be used to add the indices to the request and the indices method should also be removed from DeleteByQueryRequest.

That would make sense I agree, but these methods come from IndicesRequest and must be implemented by DeleteByQueryRequest so that request execution logic knows which indices are involved in the delete by query.

@kunal642

This comment has been minimized.

Copy link
Contributor Author

commented Feb 22, 2017

@tlrx The IndicesRequest can be removed because only the indices methods are present in that interface and the DeleteByQueryRequest class does not need those methods.
the SearchRequest which is passed to DeleteByQueryRequest can have the indices and types specified in it.

core/src/test/java/org/elasticsearch/action/bulk/byscroll/DeleteByQueryRequestTests.java Outdated
@@ -74,4 +75,23 @@ protected void extraRandomizationForSlice(DeleteByQueryRequest original) {
protected void extraForSliceAssertions(DeleteByQueryRequest original, DeleteByQueryRequest forSliced) {
// No extra assertions needed
}

@Test

This comment has been minimized.

Copy link
@javanna

javanna Feb 22, 2017

Member

the Test annotation is banned in our project :) Something should fail when running the build from gradle. if the test method starts with test we are good, no need for the annotation

@javanna

This comment has been minimized.

Copy link
Member

commented Feb 22, 2017

hi @kunal642 , the DeleteByQueryRequest needs to implement IndicesRequest as plugins may want to retrieve/replace the indices that the request applies to. IndicesRequest allows to do it in a generic manner.

@kunal642 kunal642 force-pushed the kunal642:issue#21984 branch Feb 22, 2017

@kunal642

This comment has been minimized.

Copy link
Contributor Author

commented Feb 22, 2017

@javanna removed the annotation

@kunal642 kunal642 force-pushed the kunal642:issue#21984 branch Feb 22, 2017

@elasticmachine

This comment has been minimized.

Copy link

commented Feb 23, 2017

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?

@kunal642

This comment has been minimized.

Copy link
Contributor Author

commented Feb 27, 2017

@javanna is there anything else that needs to be changed?

@javanna
Copy link
Member

left a comment

hi @kunal642 I had another look, left one comment, looks good otherwise

core/src/main/java/org/elasticsearch/action/bulk/byscroll/DeleteByQueryRequest.java Outdated
return getSearchRequest().types();
}

public IndicesRequest types(String...types) {

This comment has been minimized.

Copy link
@javanna

javanna Feb 27, 2017

Member

can you return DeleteByQueryRequest here please? Also add a whitespace between String... and types?

@kunal642

This comment has been minimized.

Copy link
Contributor Author

commented Feb 27, 2017

@javanna the requested changes have been done. Please review

@kunal642 kunal642 force-pushed the kunal642:issue#21984 branch to 084ec3b Feb 28, 2017

@kunal642

This comment has been minimized.

Copy link
Contributor Author

commented Mar 1, 2017

@javanna Review this please

@javanna
Copy link
Member

left a comment

hi @kunal642 I left a couple of comments


for (int i = 0; i < numIndices; i++) {
indices[i] = randomSimpleString(random(), 1, 30);
types[i] = randomSimpleString(random(), 1, 30);

This comment has been minimized.

Copy link
@javanna

javanna Mar 2, 2017

Member

This throws ArrayIndexOutOfBoundsException , can you fix that please?


public void testToDeleteByTypes() {
int numTypes = between(1, 50);
int numIndices = between(1, 100);

This comment has been minimized.

Copy link
@javanna

javanna Mar 2, 2017

Member

do we need to test indices as well as part of this test? Maybe only test types?

@@ -74,4 +72,22 @@ protected void extraRandomizationForSlice(DeleteByQueryRequest original) {
protected void extraForSliceAssertions(DeleteByQueryRequest original, DeleteByQueryRequest forSliced) {
// No extra assertions needed
}

public void testToDeleteByTypes() {

This comment has been minimized.

Copy link
@javanna

javanna Mar 2, 2017

Member

can you please rename to testTypesSetter or something along those lines ?

@elasticmachine

This comment has been minimized.

Copy link

commented Mar 2, 2017

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?

@kunal642

This comment has been minimized.

Copy link
Contributor Author

commented Mar 2, 2017

@javanna the requested changes have been done.

@javanna javanna added the v5.4.0 label Mar 2, 2017

@javanna

javanna approved these changes Mar 2, 2017

Copy link
Member

left a comment

thanks @kunal642 I will merge this.

return getSearchRequest().types();
}

public DeleteByQueryRequest types(String... types) {

This comment has been minimized.

Copy link
@javanna

javanna Mar 2, 2017

Member

sorry @kunal642 one last thing, could you also test this method please? set the types using this new method and check that the underlying search request types reflect the change?

@kunal642

This comment has been minimized.

Copy link
Contributor Author

commented Mar 2, 2017

@javanna done

@javanna javanna merged commit 32d292b into elastic:master Mar 2, 2017

1 check passed

CLA Commit author has signed the CLA
Details
@kunal642

This comment has been minimized.

Copy link
Contributor Author

commented Mar 2, 2017

@javanna thanks :)

javanna added a commit that referenced this pull request Mar 3, 2017

Added types options to DeleteByQueryRequest (#23265)
Add types setter and getter to `DeleteByQueryRequest`, which delegate to the inner `SearchRequest`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.