Skip to content

Commit d3c53aa

Browse files
authored
Fix reused/recovered bytes for files that are only partially recovered from cache (#95987) (#97103)
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
1 parent 6d5a26e commit d3c53aa

File tree

5 files changed

+35
-3
lines changed

5 files changed

+35
-3
lines changed

docs/changelog/95987.yaml

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
pr: 95987
2+
summary: Fix reused/recovered bytes for files that are only partially recovered from
3+
cache
4+
area: Snapshot/Restore
5+
type: bug
6+
issues:
7+
- 95970
8+
- 95994

x-pack/plugin/searchable-snapshots/src/internalClusterTest/java/org/elasticsearch/xpack/searchablesnapshots/SearchableSnapshotsIntegTests.java

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -92,7 +92,6 @@
9292

9393
public class SearchableSnapshotsIntegTests extends BaseSearchableSnapshotsIntegTestCase {
9494

95-
@AwaitsFix(bugUrl = "https://github.com/elastic/elasticsearch/pull/95987")
9695
public void testCreateAndRestoreSearchableSnapshot() throws Exception {
9796
final String fsRepoName = randomAlphaOfLength(10);
9897
final String indexName = randomAlphaOfLength(10).toLowerCase(Locale.ROOT);

x-pack/plugin/searchable-snapshots/src/internalClusterTest/java/org/elasticsearch/xpack/searchablesnapshots/recovery/SearchableSnapshotRecoveryStateIntegrationTests.java

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,6 @@ protected Collection<Class<? extends Plugin>> nodePlugins() {
6161
return CollectionUtils.appendToCopy(super.nodePlugins(), TestRepositoryPlugin.class);
6262
}
6363

64-
@AwaitsFix(bugUrl = "https://github.com/elastic/elasticsearch/issues/95994")
6564
public void testRecoveryStateRecoveredBytesMatchPhysicalCacheState() throws Exception {
6665
final String fsRepoName = randomAlphaOfLength(10);
6766
final String indexName = randomAlphaOfLength(10).toLowerCase(Locale.ROOT);

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

Lines changed: 20 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -506,13 +506,32 @@ private void prewarmCache(ActionListener<Void> listener, Supplier<Boolean> cance
506506

507507
final AtomicLong prefetchedBytes = new AtomicLong(0L);
508508
try (var fileListener = new RefCountingListener(ActionListener.runBefore(completionListener.acquire().map(v -> {
509+
// we don't support files to be reported as partially recovered from disk and partially from the blob store, but
510+
// this is something that can happen for fully mounted searchable snapshots. It is possible that prewarming
511+
// prefetched nothing if a concurrent search was executing (and cached the data) or if the data were fetched from
512+
// the blob cache system index.
509513
if (prefetchedBytes.get() == 0L) {
510514
recoveryState.markIndexFileAsReused(file.physicalName());
511515
} else {
512-
recoveryState.getIndex().addRecoveredFromSnapshotBytesToFile(file.physicalName(), prefetchedBytes.get());
516+
recoveryState.getIndex().addRecoveredFromSnapshotBytesToFile(file.physicalName(), file.length());
513517
}
514518
return v;
515519
}), () -> IOUtils.closeWhileHandlingException(input)))) {
520+
521+
if (input instanceof CachedBlobContainerIndexInput cachedIndexInput) {
522+
if (cachedIndexInput.getPersistentCacheInitialLength() == file.length()) {
523+
logger.trace(
524+
() -> format(
525+
"%s file [%s] is already available in cache (%d bytes)",
526+
shardId,
527+
file.physicalName(),
528+
file.length()
529+
)
530+
);
531+
continue;
532+
}
533+
}
534+
516535
for (int p = 0; p < file.numberOfParts(); p++) {
517536
final int part = p;
518537
prewarmTaskRunner.enqueueTask(fileListener.acquire().map(releasable -> {

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

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -127,6 +127,13 @@ protected void readWithoutBlobCache(ByteBuffer b) throws Exception {
127127
assert bytesRead == length : bytesRead + " vs " + length;
128128
}
129129

130+
/**
131+
* @return Returns the number of bytes already cached for the file in the cold persistent cache
132+
*/
133+
public long getPersistentCacheInitialLength() throws Exception {
134+
return cacheFileReference.get().getInitialLength();
135+
}
136+
130137
/**
131138
* Prefetches a complete part and writes it in cache. This method is used to prewarm the cache.
132139
*

0 commit comments

Comments
 (0)