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

Update elasticsearch.modules.parent-join.internalClusterTest #102189

Merged
merged 13 commits into from
Dec 9, 2023
Merged

Update elasticsearch.modules.parent-join.internalClusterTest #102189

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
72bb148
Fix TokenCountFieldMapperIntegrationIT
JVerwolf Nov 14, 2023
1a190fb
Fix ChildQuerySearchIT
JVerwolf Nov 14, 2023
ed6bbbc
Destroy formatting with spotless
JVerwolf Nov 14, 2023
12e8626
Revert changes
JVerwolf Nov 14, 2023
25b4ae5
Remove vestigial test limbs
JVerwolf Nov 14, 2023
340cb6a
Merge branch 'main' into fix-test-memory-management-parent-join
elasticmachine Nov 15, 2023
0ab8fd6
better abstraction for assertScrollResponses
JVerwolf Nov 15, 2023
6597e47
Destroy formatting with spotless
JVerwolf Nov 15, 2023
096eea2
Fix logic in assertScrollResponses
JVerwolf Nov 15, 2023
dcf93aa
Merge branch 'main' of github.com:elastic/elasticsearch into fix-test…
JVerwolf Dec 1, 2023
0972a1c
PR Feedback
JVerwolf Dec 1, 2023
fa4ec03
Spotless
JVerwolf Dec 1, 2023
a1f9a9f
Update documentation
JVerwolf Dec 2, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
86 changes: 34 additions & 52 deletions ...nt-join/src/internalClusterTest/java/org/elasticsearch/join/query/ChildQuerySearchIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@
import org.elasticsearch.action.index.IndexRequestBuilder;
import org.elasticsearch.action.search.SearchPhaseExecutionException;
import org.elasticsearch.action.search.SearchRequestBuilder;
import org.elasticsearch.action.search.SearchResponse;
import org.elasticsearch.action.search.SearchType;
import org.elasticsearch.action.support.WriteRequest.RefreshPolicy;
import org.elasticsearch.common.lucene.search.function.CombineFunction;
Expand Down Expand Up @@ -64,11 +63,13 @@
import static org.elasticsearch.join.query.JoinQueryBuilders.hasParentQuery;
import static org.elasticsearch.join.query.JoinQueryBuilders.parentId;
import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertAcked;
import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertFailures;
import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertHitCount;
import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertHitCountAndNoFailures;
import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertNoFailures;
import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertNoFailuresAndResponse;
import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertResponse;
import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertScrollResponses;
import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertSearchHit;
import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertSearchHits;
import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.hasId;
Expand Down Expand Up @@ -1306,40 +1307,23 @@ public void testParentChildQueriesNoParentType() throws Exception {
client().prepareIndex("test").setId(parentId).setSource("p_field", "1").get();
refresh();

try {
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we need to fix these? For the exception case the production code should handle any ref counting?
Maybe a cleanup best left for another PR since it's unrelated to this effort?

prepareSearch("test").setQuery(hasChildQuery("child", termQuery("c_field", "1"), ScoreMode.None)).get();
fail();
} catch (SearchPhaseExecutionException e) {
assertThat(e.status(), equalTo(RestStatus.BAD_REQUEST));
}

try {
prepareSearch("test").setQuery(hasChildQuery("child", termQuery("c_field", "1"), ScoreMode.Max)).get();
fail();
} catch (SearchPhaseExecutionException e) {
assertThat(e.status(), equalTo(RestStatus.BAD_REQUEST));
}

try {
prepareSearch("test").setPostFilter(hasChildQuery("child", termQuery("c_field", "1"), ScoreMode.None)).get();
fail();
} catch (SearchPhaseExecutionException e) {
assertThat(e.status(), equalTo(RestStatus.BAD_REQUEST));
}

try {
prepareSearch("test").setQuery(hasParentQuery("parent", termQuery("p_field", "1"), true)).get();
fail();
} catch (SearchPhaseExecutionException e) {
assertThat(e.status(), equalTo(RestStatus.BAD_REQUEST));
}

try {
prepareSearch("test").setPostFilter(hasParentQuery("parent", termQuery("p_field", "1"), false)).get();
fail();
} catch (SearchPhaseExecutionException e) {
assertThat(e.status(), equalTo(RestStatus.BAD_REQUEST));
}
assertFailures(
prepareSearch("test").setQuery(hasChildQuery("child", termQuery("c_field", "1"), ScoreMode.None)),
RestStatus.BAD_REQUEST
);
assertFailures(
prepareSearch("test").setQuery(hasChildQuery("child", termQuery("c_field", "1"), ScoreMode.Max)),
RestStatus.BAD_REQUEST
);
assertFailures(
prepareSearch("test").setPostFilter(hasChildQuery("child", termQuery("c_field", "1"), ScoreMode.None)),
RestStatus.BAD_REQUEST
);
assertFailures(prepareSearch("test").setQuery(hasParentQuery("parent", termQuery("p_field", "1"), true)), RestStatus.BAD_REQUEST);
assertFailures(
prepareSearch("test").setPostFilter(hasParentQuery("parent", termQuery("p_field", "1"), false)),
RestStatus.BAD_REQUEST
);
}

public void testParentChildCaching() throws Exception {
Expand Down Expand Up @@ -1405,23 +1389,21 @@ public void testParentChildQueriesViaScrollApi() throws Exception {
boolQuery().must(matchAllQuery()).filter(hasParentQuery("parent", matchAllQuery(), false)) };

for (QueryBuilder query : queries) {
SearchResponse scrollResponse = prepareSearch("test").setScroll(TimeValue.timeValueSeconds(30))
.setSize(1)
.addStoredField("_id")
.setQuery(query)
.execute()
.actionGet();

assertNoFailures(scrollResponse);
assertThat(scrollResponse.getHits().getTotalHits().value, equalTo(10L));
int scannedDocs = 0;
do {
assertThat(scrollResponse.getHits().getTotalHits().value, equalTo(10L));
scannedDocs += scrollResponse.getHits().getHits().length;
scrollResponse = client().prepareSearchScroll(scrollResponse.getScrollId()).setScroll(TimeValue.timeValueSeconds(30)).get();
} while (scrollResponse.getHits().getHits().length > 0);
clearScroll(scrollResponse.getScrollId());
assertThat(scannedDocs, equalTo(10));
assertScrollResponses(
Copy link
Member

Choose a reason for hiding this comment

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

Do we need the timeout here? Can we maybe just remove the timeout from this test to not have to bother with timeouts? (those are a known tricky issue around ref counting actually ...)

Copy link
Contributor Author

@JVerwolf JVerwolf Dec 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

The search request builder requires it (unless I'm mistaken), though mabye I can create the search directly so that I don't have to refactor the builder. I could probably change this with a broader refactor, but isn't timeouts something we have to handle anyways?

prepareSearch("test").setScroll(TimeValue.timeValueSeconds(30)).setSize(1).addStoredField("_id").setQuery(query),
responses -> {

responses.forEach(searchResponse -> {
assertNoFailures(searchResponse);
assertThat(searchResponse.getHits().getTotalHits().value, equalTo(10L));
});

var scannedDocs = responses.stream()
.map(searchResponse -> searchResponse.getHits().getHits().length)
.reduce(0, Integer::sum);
assertThat(scannedDocs, equalTo(10));
}
);
}
}

Expand Down
2 changes: 1 addition & 1 deletion test/framework/src/main/java/org/elasticsearch/test/ESIntegTestCase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1878,7 +1878,7 @@ protected void addError(Exception e) {
/**
* Clears the given scroll Ids
*/
public void clearScroll(String... scrollIds) {
public static void clearScroll(String... scrollIds) {
ClearScrollResponse clearResponse = client().prepareClearScroll().setScrollIds(Arrays.asList(scrollIds)).get();
assertThat(clearResponse.isSucceeded(), equalTo(true));
}
Expand Down
35 changes: 32 additions & 3 deletions test/framework/src/main/java/org/elasticsearch/test/hamcrest/ElasticsearchAssertions.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
import org.elasticsearch.search.SearchHit;
import org.elasticsearch.search.suggest.Suggest;
import org.elasticsearch.test.NotEqualMessageBuilder;
import org.elasticsearch.transport.TransportMessage;
import org.elasticsearch.xcontent.ToXContent;
import org.elasticsearch.xcontent.XContentBuilder;
import org.elasticsearch.xcontent.XContentParser;
Expand All @@ -49,6 +50,7 @@
import java.io.IOException;
import java.nio.file.Files;
import java.nio.file.Path;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Iterator;
import java.util.List;
Expand All @@ -62,6 +64,8 @@

import static org.apache.lucene.tests.util.LuceneTestCase.expectThrows;
import static org.apache.lucene.tests.util.LuceneTestCase.expectThrowsAnyOf;
import static org.elasticsearch.test.ESIntegTestCase.clearScroll;
import static org.elasticsearch.test.ESIntegTestCase.client;
import static org.elasticsearch.test.LambdaMatchers.transformedArrayItemsMatch;
import static org.elasticsearch.test.LambdaMatchers.transformedItemsMatch;
import static org.elasticsearch.test.LambdaMatchers.transformedMatch;
Expand All @@ -71,6 +75,7 @@
import static org.hamcrest.Matchers.arrayContaining;
import static org.hamcrest.Matchers.arrayWithSize;
import static org.hamcrest.Matchers.containsInAnyOrder;
import static org.hamcrest.Matchers.containsString;
import static org.hamcrest.Matchers.empty;
import static org.hamcrest.Matchers.emptyArray;
import static org.hamcrest.Matchers.greaterThan;
Expand Down Expand Up @@ -364,6 +369,26 @@ public static void assertResponse(ActionRequestBuilder<?, SearchResponse> search
}
}

public static void assertScrollResponses(SearchRequestBuilder searchRequestBuilder, Consumer<List<SearchResponse>> consumer) {
var timoutSeconds = 30;
Copy link
Contributor

@iverase iverase Nov 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This is an interesting approach. Could you check if you can use this method with the test in #102053?

Maybe the timeout should be an input to the method?

Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW: This method should be able to handle the cases in TransportTwoNodesSearchIT which I am trying to work out. WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @iverase, I'll take a look. Sure, I can make the timeout an input.

I tried a few different approaches, but settled on making the consumer accept the list of partial responses. That way, the caller can decide if they want to validate individual aspects of the the response and/or the whole thing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Check out 0ab8fd6

This updates the test you linked.

I changed assertScrollResponses to return a record of "slices" on the result list. This now makes it easy for the tests to iterate over the the various aspects of the Scroll and perform thier assertions without duplicating the slicing logic each time.

searchRequestBuilder.setScroll(TimeValue.timeValueSeconds(timoutSeconds));
List<SearchResponse> responses = new ArrayList<>();
var scrollResponse = searchRequestBuilder.get();
responses.add(scrollResponse);
try {
while (scrollResponse.getHits().getHits().length > 0) {
scrollResponse = client().prepareSearchScroll(scrollResponse.getScrollId())
Copy link
Contributor

Choose a reason for hiding this comment

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

Lets add to ESIngestTestCase a method so we don't have to create the query here. Something like:

 public static SearchScrollRequestBuilder prepareScrollSearch(String scrollId, TimeValue timeout) {
        return client().prepareSearchScroll(scrollId).setScroll(timeout);
   }

.setScroll(TimeValue.timeValueSeconds(timoutSeconds))
.get();
responses.add(scrollResponse);
}
consumer.accept(responses);
clearScroll(scrollResponse.getScrollId());
Copy link
Contributor

Choose a reason for hiding this comment

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

This should go in the finally block?

} finally {
responses.forEach(TransportMessage::decRef);
Copy link
Contributor

Choose a reason for hiding this comment

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

nit:

responses.forEach(SearchResponse::decRef);

So we dont need an extra import

}
}

public static void assertResponse(ActionFuture<SearchResponse> responseFuture, Consumer<SearchResponse> consumer)
throws ExecutionException, InterruptedException {
var res = responseFuture.get();
Expand Down Expand Up @@ -437,6 +462,10 @@ public static void assertFailures(SearchRequestBuilder searchRequestBuilder, Res
}
}

public static void assertFailures(SearchRequestBuilder searchRequestBuilder, RestStatus restStatus) {
assertFailures(searchRequestBuilder, restStatus, containsString(""));
}

public static void assertNoFailures(BaseBroadcastResponse response) {
if (response.getFailedShards() != 0) {
final AssertionError assertionError = new AssertionError("[" + response.getFailedShards() + "] shard failures");
Expand Down Expand Up @@ -786,9 +815,9 @@ public static void assertToXContentEquivalent(BytesReference expected, BytesRefe
* Often latches are called as <code>assertTrue(latch.await(1, TimeUnit.SECONDS));</code>
* In case of a failure this will just throw an assertion error without any further message
*
* @param latch The latch to wait for
* @param timeout The value of the timeout
* @param unit The unit of the timeout
* @param latch The latch to wait for
* @param timeout The value of the timeout
* @param unit The unit of the timeout
* @throws InterruptedException An exception if the waiting is interrupted
*/
public static void awaitLatch(CountDownLatch latch, long timeout, TimeUnit unit) throws InterruptedException {
Expand Down