Fix reused/recovered bytes for files that are only partially recovered from cache

[tlrx]

Before #95891 a file was considered as reused in recovery if
it was fully cached in the cold persistent cache. Otherwise
the full file length was reported as recovered from the
blob store during prewarming.

While working on #95891 I changed the
CachedBlobContainerIndexInput.prefetchPart()
method to be more precise on the number of bytes that were
effectively read from the blob store during prewarming.

But this obviously broke some tests (#95970) because a file
cannot be partially recovered from disk and from remote. This
change restores the previous behavior with one adjustment:
the file is considered as reused if the prewarm method
effectively prefetched 0 bytes.

Closes #95970
Closes #95994

[elasticsearchmachine]

Hi @tlrx, I've created a changelog YAML for you.

[elasticsearchmachine]

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

[idegtiarenko]

Is there a chance this could break for a very small segment files?
I am thinking about example where 1/3 parts were in disk already and we only downloaded 2/3.

I wonder if we should consider changins boolean reused to long reused in FileDetail and consider a file fully recovered when recovered < length && reused + recovered == length?

[tlrx]

I'm not sure to understand why it would break for small files? We're comparing the number of bytes prewarmed with the total file length so the number of parts do not really matter. Also, most small files are likely to be either fully prewarmed or fully reused.

I wonder if we should consider changins boolean reused to long reused in FileDetail and consider a file fully recovered when recovered < length && reused + recovered == length?

We could but this is a larger change that I don't think is really worth doing since it only makes sense for cold cache.

[idegtiarenko]

I am trying to understand if there is something that guarantee that the following would not happen:

• 2/3 are already cached locally and would not be downloaded
• 1/3 is downloaded updating a stats so that it expects something would complete download before reporting it as finished.

[idegtiarenko]

Is it worth adding an assertion somewhere around

elasticsearch/x-pack/plugin/searchable-snapshots/src/main/java/org/elasticsearch/xpack/searchablesnapshots/store/SearchableSnapshotDirectory.java

Line 477 in bb3ef55

recoveryState.setPreWarmComplete();

that would verify that for each index for each FileDetail we have assert reused ^ recovered == length (possibly in a separate pr)?

[elasticsearchmachine]

Hi @tlrx, I've updated the changelog YAML for you.

[tlrx]

@elasticmachine retest this please

[tlrx]

Thanks @idegtiarenko for your feedback. I reworked this change to revert to the previous behavior.

Is it worth adding an assertion somewhere around [..] that would verify that for each index for each FileDetail we have assert reused ^ recovered == length (possibly in a separate pr)?

I suppose we can yes. We don't have must assertions around recovery file details but I can add one as a follow up.

[tlrx]

@elasticmachine run elasticsearch-ci/part-1

[tlrx]

Thanks Ievgen!

[elasticsearchmachine]

💚 Backport successful

Status	Branch	Result
✅	8.9
✅	8.8