Add index commit id to searcher

[dnhatn]

This commit adds an id, which is composed of the ids of segment files, to ElasticsearchDirectoryReader. With this id, we can retry search requests on another shard copy if its latest "snapshot" has the same segment files as the failing shard.

[dnhatn]

I am labeling this WIP, although it's ready as I am not sure the approach in the PR. The way we compose the searcher id might be too aggressive. If we need to improve search resiliency for searchable snapshots only, then we can use the commit id instead.

[elasticmachine]

Pinging @elastic/es-distributed (:Distributed/Engine)

[henningandersen]

I believe there are two goals we could have with this id:

1. The ability to repeat the fetch phase if it fails. Here we need identical doc-ids on the replica.
2. The ability to continue to use a PIT despite failures in the cluster. Here we do not need identical doc-ids.

I wonder if the second part is better served by maxSeqNo instead whenever gcp==maxSeqNo? Plus primary term for correctness.

It might be two completely separate ids and thus handled in another PR. I only wanted to raise this here to gather consensus that we cannot come up with a single id or failover scheme that can serve both purposes.

[henningandersen]

It is not clear to me that this is valid across different shard copies. The id generation starts somewhere random and then increments. I acknowledge the risk is small and I did not dig deeply into whether this increases the risk of collissions over using more standard uuid generation.

[dnhatn]

@jimczi If we implement #56828 using a composition of indexUUID, shardId, and sequence number (instead of internal document ID), then PIT can go with Henning's option-2. WDYT?

[dnhatn]

@henningandersen @jimczi

We (Jim, Francisco, and I) discussed this PR and agreed to add two IDs (i.e., commitID and seqID) to searchers. I implemented the commitID using an external UUID that is generated when an engine flushes (see 2f410eb). However, I found that using an external UUID isn't better than the id of an index commit. Hence, I re-implemented it using the id of an index commit. Would you please take a look? Thank you!

[dnhatn]

Could we also set replicas=1 and then check that the recovered copy has the same commit id?

We flush after copying segment files in peer recovery to associate a new translog UUID, and this flush changes the commit id.

We can avoid this limitation by having an external commit id, and change it whenever the InternalEngine performs a flush. I implemented this, but I reverted it as it helps only this situation. I am happy to bring it back if you prefer.

[henningandersen]

but I reverted it as it helps only this situation

Is the purpose of the change not that we can fail over a search to another replica copy containing the same data? I suppose the approach taken here could work for searchable snapshots, but it would be nice to have an approach that also works for shards that already went through file based peer recovery and did not see any changes since (like most warm indices today). Or is there a good reason that we cannot/should not handle that scenario?

[s1monw]

LGTM

[dnhatn]

Is the purpose of the change not that we can fail over a search to another replica copy containing the same data?

Yes, that's the original purpose of this PR.

I suppose the approach taken here could work for searchable snapshots.

Yes, it works with searchable snapshots.

It would be nice to have an approach that also works for shards that already went through file based peer recovery and did not see any changes since (like most warm indices today). Or is there a good reason that we cannot/should not handle that scenario?

The problem is that we flush to create a new index commit to associate with a new local translog on replicas after performing a file-based recovery. This generates a new commit_id, although the content of the index commit doesn't change. To circumvent this, we can have an external commit id that doesn't change in file-based recoveries (please see 5ec5781).

[henningandersen]

Thanks @dnhatn , I think we should go for the external commit id like you did in 5ec5781.

[henningandersen]

Chatted with @dnhatn about the main issue of the external commit id: relying on lucene not merging when associating a new translog with the commit. In order to continue this path of development, let us stick to the approach you have here (using the commit id of the segment infos), or revisit your original proposal of relying on individual segment commit-ids.

[dnhatn]

Thanks everyone for reviewing. I am merging this PR as is and will revisit the previous approach later.