Address CompoundRetrieverBuilder Failure Handling

[john-wagster]

exploring options for improving error handling for CompoundRetrieverBuilder particularly in the case where a response of sub retriever is 2xx but some shard failed to retrieve data.

addresses: #136529

[john-wagster]

@pmpailis curious if this is roughly what you were thinking in terms of improving error handling and if you had other thoughts about how this should behave or additional checks. What else would be nice to have here? I thought about enhancing the error message handling within the if (false == failures.isEmpty()) { block to include a list of the failure messages as well. How do you feel about that?

[pmpailis]

Thanks @john-wagster for picking this up! ❤️ Yeah, this is pretty much what I was thinking as well; only minor comments on the type of exception that we would throw (to avoid constantly 5xx).

Will take a look at adding a test case as well.

[elasticsearchmachine]

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

[elasticsearchmachine]

Hi @john-wagster, I've created a changelog YAML for you.

[john-wagster]

@pmpailis I created a mock test which helped with cleaning up the code. Let me know what you think about that and the current state of the code. I also realized i have no idea what version labels this should actually be applied to so I just put them all on here. I think that's everything that's currently supported??? Thoughts on what releases this should target would be welcome.

[pmpailis]

Thanks @john-wagster ! This looks really nice! In addition to using mocks, we could also use a custom query through a test plugin and using it as part of the integration tests (e..g in LinearRetrieverIT#nodePlugins, RRFRetrieverBuilderIT#nodePlugins, etc).

E.g.

private static class ShardFailingQueryBuilder extends AbstractQueryBuilder<ShardFailingQueryBuilder> {
        private static final String NAME = "shard_failing_query";

        private static ShardFailingQueryBuilder fromXContent(XContentParser parser) {
            return new ShardFailingQueryBuilder();
        }

        ShardFailingQueryBuilder() {}

        ShardFailingQueryBuilder(StreamInput in) throws IOException {
            super(in);
        }

        @Override
        public String getWriteableName() {
            return NAME;
        }

        @Override
        public TransportVersion getMinimalSupportedVersion() {
            return TransportVersion.current();
        }

        @Override
        protected void doWriteTo(StreamOutput out) throws IOException {

        }

        @Override
        protected void doXContent(XContentBuilder builder, Params params) throws IOException {
            builder.startObject(NAME);
            builder.endObject();
        }

        @Override
        protected Query doToQuery(SearchExecutionContext context) throws IOException {
            if(frequently() && context.getShardId() % 2 == 0) {
                throw new IllegalArgumentException("simulated failure");
            }else{
                return new MatchAllDocsQuery();
            }
        }

        @Override
        protected boolean doEquals(ShardFailingQueryBuilder other) {
            return true;
        }

        @Override
        protected int doHashCode() {
            return 0;
        }
    }

    public static class FailingQueryPlugin extends Plugin implements SearchPlugin {
        public FailingQueryPlugin() {
        }

        @Override
        public List<QuerySpec<?>> getQueries() {
            return List.of(new QuerySpec<QueryBuilder>(ShardFailingQueryBuilder.NAME, ShardFailingQueryBuilder::new, ShardFailingQueryBuilder::fromXContent));
        }
    }

And then have a test like:

    public void testLinearInnerRetrieverPartialSearchErrors() {
        final int rankWindowSize = 100;
        SearchSourceBuilder source = new SearchSourceBuilder();
        StandardRetrieverBuilder standard0 = new StandardRetrieverBuilder(new ShardFailingQueryBuilder());
        StandardRetrieverBuilder standard1 = new StandardRetrieverBuilder(new MatchAllQueryBuilder());
        source.retriever(
            new LinearRetrieverBuilder(
                Arrays.asList(
                    new CompoundRetrieverBuilder.RetrieverSource(standard0, null),
                    new CompoundRetrieverBuilder.RetrieverSource(standard1, null)
                ),
                rankWindowSize
            )
        );
        SearchRequestBuilder req = client().prepareSearch(INDEX).setSource(source);
        var resp = req.get();          

[elasticsearchmachine]

Hi @john-wagster, I've updated the changelog YAML for you.

[benwtrent]

Hey, by default, we allow partial results and return a 2xx. Does this break that? Meaning, if there is a failed shard, do we still return a 2xx by default?

We should:

• Allow partial results
• If partial results are desired, we should indicate in the final result that some shards failed, and return 2xx
• If partial results are NOT desired, we should return something other than 2xx
• If all shards failed, we should not return a 2xx and indicate the failure