Update elasticsearch.modules.parent-join.internalClusterTest

[JVerwolf]

Part of the broader work covered in #102030

Updates tests in:

• ChildQuerySearchIT
• TokenCountFieldMapperIntegrationIT

[elasticsearchmachine]

Pinging @elastic/es-search (Team:Search)

[JVerwolf]

@elasticmachine update branch

[iverase]

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?

[iverase]

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

[JVerwolf]

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.

[JVerwolf]

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.

[iverase]

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);
   }

[iverase]

Let's use a TimeValue here.

[iverase]

This should go in the finally block?

[iverase]

nit:

responses.forEach(SearchResponse::decRef);

So we dont need an extra import

[iverase]

I have a fundamental concern with your approach as some test might want to perform actions between scroll calls instead of asserting after all queries have been done. I wonder if something more specialize would be better here, something on these lines:

  public static void assertTotalHitsAndResponses(
        TimeValue keepAlive,
        SearchRequestBuilder searchRequestBuilder,
        Consumer<SearchResponse> initialSearchResponse,
        Consumer<SearchResponse> scrollResponses,
        int totalHits
    ) {
        searchRequestBuilder.setScroll(keepAlive);
        List<SearchResponse> responses = new ArrayList<>();
        var scrollResponse = searchRequestBuilder.get();
        responses.add(scrollResponse);
        int countDocs = 0;
        try {
            assertThat(scrollResponse.getHits().getTotalHits().value, equalTo((long) totalHits));
            countDocs += scrollResponse.getHits().getHits().length;
            initialSearchResponse.accept(scrollResponse);
            while (scrollResponse.getHits().getHits().length > 0) {
                scrollResponse = prepareScrollSearch(scrollResponse.getScrollId(), keepAlive).get();
                responses.add(scrollResponse);
                assertThat(scrollResponse.getHits().getTotalHits().value, equalTo((long) totalHits));
                countDocs += scrollResponse.getHits().getHits().length;
                scrollResponses.accept(scrollResponse);
            }
        } finally {
            clearScroll(scrollResponse.getScrollId());
            responses.forEach(SearchResponse::decRef);
        }
        assertThat(countDocs, equalTo(totalHits));
    }

[original-brownbear]

2 questions, sorry it took me so long to get to this one!

[original-brownbear]

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?

[original-brownbear]

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 ...)

[JVerwolf]

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?

[JVerwolf]

@iverase

I have a fundamental concern with your approach as some test might want to perform actions between scroll calls instead of asserting after all queries have been done. I wonder if something more specialize would be better here, something on these lines:

  public static void assertTotalHitsAndResponses(
        TimeValue keepAlive,
        SearchRequestBuilder searchRequestBuilder,
        Consumer<SearchResponse> initialSearchResponse,
        Consumer<SearchResponse> scrollResponses,
        int totalHits
    ) {
        searchRequestBuilder.setScroll(keepAlive);
        List<SearchResponse> responses = new ArrayList<>();
        var scrollResponse = searchRequestBuilder.get();
        responses.add(scrollResponse);
        int countDocs = 0;
        try {
            assertThat(scrollResponse.getHits().getTotalHits().value, equalTo((long) totalHits));
            countDocs += scrollResponse.getHits().getHits().length;
            initialSearchResponse.accept(scrollResponse);
            while (scrollResponse.getHits().getHits().length > 0) {
                scrollResponse = prepareScrollSearch(scrollResponse.getScrollId(), keepAlive).get();
                responses.add(scrollResponse);
                assertThat(scrollResponse.getHits().getTotalHits().value, equalTo((long) totalHits));
                countDocs += scrollResponse.getHits().getHits().length;
                scrollResponses.accept(scrollResponse);
            }
        } finally {
            clearScroll(scrollResponse.getScrollId());
            responses.forEach(SearchResponse::decRef);
        }
        assertThat(countDocs, equalTo(totalHits));
    }

Let's go a step further and allow the user to decide which request they want to interact with, and in doing so, simplify the api. I've updated the code to this effect. WDYT?

[iverase]

LGTM

[original-brownbear]

LGTM, hope you don't mind me just merging this John! Looks good and removes some noise from my work on this :) Thanks!