Skip to content

Commit 4b72136

Browse files
authored
Fix reused/recovered bytes for files that are only partially recovered from cache (elastic#95987) (elastic#97102)
Before elastic#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 elastic#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 (elastic#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 elastic#95970 Closes elastic#95994
1 parent 018e362 commit 4b72136

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
@@ -94,7 +94,6 @@
9494

9595
public class SearchableSnapshotsIntegTests extends BaseSearchableSnapshotsIntegTestCase {
9696

97-
@AwaitsFix(bugUrl = "https://github.com/elastic/elasticsearch/pull/95987")
9897
public void testCreateAndRestoreSearchableSnapshot() throws Exception {
9998
final String fsRepoName = randomAlphaOfLength(10);
10099
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
@@ -122,6 +122,13 @@ protected void readWithoutBlobCache(ByteBuffer b) throws Exception {
122122
assert bytesRead == length : bytesRead + " vs " + length;
123123
}
124124

125+
/**
126+
* @return Returns the number of bytes already cached for the file in the cold persistent cache
127+
*/
128+
public long getPersistentCacheInitialLength() throws Exception {
129+
return cacheFileReference.get().getInitialLength();
130+
}
131+
125132
/**
126133
* Prefetches a complete part and writes it in cache. This method is used to prewarm the cache.
127134
*

0 commit comments

Comments
 (0)