Added decRef calls to SearchResponse tests

[quux00]

Tests covered these sections of the ES repo
x-pack/plugin/monitoring
x-pack/plugin/rank-rrf
x-pack/plugin/rollup
x-pack/plugin/search-business-rules

The decRef was either added explicitly or implicitly by using ElasticsearchAssertions.assertResponse.

[elasticsearchmachine]

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

[original-brownbear]

Looks just fine, just one (I think) actual tiny inaccuracy and one nit :)

[original-brownbear]

Why not use the assertResponse pattern here?

[quux00]

I tried, but when I did that, the assertMonitoringDoc method (currently line 184) could no longer be resolved for reasons that weren't clear to me. And (more minor) toMap said it threw an unhandled Exception, so it would have been clunky to handle as well.

[original-brownbear]

Fair point lets leave it this way :)

[iverase]

You can use assertCheckedResponse and then you don't need to manually do it in the test.

[quux00]

Excellent. Thanks for the tip.

[original-brownbear]

Wouldn't we have to use setAndGet here and release whatever non-null response that might already be in the reference ?

[quux00]

We could do that for safety, but I don't think it's necessary (?). The AtomicReference.set call should only be invoked once, since the assertBusy "loop" stops only after none of the asserts throw an Exception. I catch the Exception above and decRef those failed SearchResponses and then re-throw, so the setter should only be called once when the assertBusy finally returns without error.

So I could add your idea for safety or put in an assert that the searchResponseRef is null before setting it.

[iverase]

I don't understand what we need all of this. I think this should be enough:

        whenExportersAreReady(() -> {
            assertBusy(() -> {
                assertCheckedResponse(client().prepareSearch(".monitoring-es-*")
                        .setCollapse(new CollapseBuilder("type"))
                        .addSort("timestamp", SortOrder.DESC) , response -> {
                        assertThat(response.status(), is(RestStatus.OK));
                        assertThat(
                            "Expecting a minimum number of 6 docs, one per collector",
                            response.getHits().getHits().length,
                            greaterThanOrEqualTo(6)
                        );

                        for (final SearchHit hit : response.getHits()) {
                            final Map<String, Object> searchHit = toMap(hit);
                            assertMonitoringDoc(searchHit, MonitoredSystem.ES, MonitoringService.MIN_INTERVAL);
                        }
                    });
            });
        });

[original-brownbear]

LGTM, sorry I reviewed this with ?w=1 which made this mostly an easy exercise but made me misread the atomic ref bit :) -> all good, thanks!

[iverase]

LGTM