From e4ff56739686a67969d86906e0efc705d5514171 Mon Sep 17 00:00:00 2001 From: Tanguy Leroux Date: Mon, 26 Jun 2023 13:28:52 +0200 Subject: [PATCH] Fix reused/recovered bytes for files that are only partially recovered from cache (#95987) 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 --- docs/changelog/95987.yaml | 8 +++++++ .../SearchableSnapshotsIntegTests.java | 1 - ...SnapshotRecoveryStateIntegrationTests.java | 1 - .../store/SearchableSnapshotDirectory.java | 21 ++++++++++++++++++- .../input/CachedBlobContainerIndexInput.java | 7 +++++++ 5 files changed, 35 insertions(+), 3 deletions(-) create mode 100644 docs/changelog/95987.yaml diff --git a/docs/changelog/95987.yaml b/docs/changelog/95987.yaml new file mode 100644 index 0000000000000..91ccbf82abce5 --- /dev/null +++ b/docs/changelog/95987.yaml @@ -0,0 +1,8 @@ +pr: 95987 +summary: Fix reused/recovered bytes for files that are only partially recovered from + cache +area: Snapshot/Restore +type: bug +issues: + - 95970 + - 95994 diff --git a/x-pack/plugin/searchable-snapshots/src/internalClusterTest/java/org/elasticsearch/xpack/searchablesnapshots/SearchableSnapshotsIntegTests.java b/x-pack/plugin/searchable-snapshots/src/internalClusterTest/java/org/elasticsearch/xpack/searchablesnapshots/SearchableSnapshotsIntegTests.java index 68e490879084e..1e39b103f1660 100644 --- a/x-pack/plugin/searchable-snapshots/src/internalClusterTest/java/org/elasticsearch/xpack/searchablesnapshots/SearchableSnapshotsIntegTests.java +++ b/x-pack/plugin/searchable-snapshots/src/internalClusterTest/java/org/elasticsearch/xpack/searchablesnapshots/SearchableSnapshotsIntegTests.java @@ -92,7 +92,6 @@ public class SearchableSnapshotsIntegTests extends BaseSearchableSnapshotsIntegTestCase { - @AwaitsFix(bugUrl = "https://github.com/elastic/elasticsearch/pull/95987") public void testCreateAndRestoreSearchableSnapshot() throws Exception { final String fsRepoName = randomAlphaOfLength(10); final String indexName = randomAlphaOfLength(10).toLowerCase(Locale.ROOT); diff --git a/x-pack/plugin/searchable-snapshots/src/internalClusterTest/java/org/elasticsearch/xpack/searchablesnapshots/recovery/SearchableSnapshotRecoveryStateIntegrationTests.java b/x-pack/plugin/searchable-snapshots/src/internalClusterTest/java/org/elasticsearch/xpack/searchablesnapshots/recovery/SearchableSnapshotRecoveryStateIntegrationTests.java index ff1967c767e2b..1530569b97524 100644 --- a/x-pack/plugin/searchable-snapshots/src/internalClusterTest/java/org/elasticsearch/xpack/searchablesnapshots/recovery/SearchableSnapshotRecoveryStateIntegrationTests.java +++ b/x-pack/plugin/searchable-snapshots/src/internalClusterTest/java/org/elasticsearch/xpack/searchablesnapshots/recovery/SearchableSnapshotRecoveryStateIntegrationTests.java @@ -61,7 +61,6 @@ protected Collection> nodePlugins() { return CollectionUtils.appendToCopy(super.nodePlugins(), TestRepositoryPlugin.class); } - @AwaitsFix(bugUrl = "https://github.com/elastic/elasticsearch/issues/95994") public void testRecoveryStateRecoveredBytesMatchPhysicalCacheState() throws Exception { final String fsRepoName = randomAlphaOfLength(10); final String indexName = randomAlphaOfLength(10).toLowerCase(Locale.ROOT); diff --git a/x-pack/plugin/searchable-snapshots/src/main/java/org/elasticsearch/xpack/searchablesnapshots/store/SearchableSnapshotDirectory.java b/x-pack/plugin/searchable-snapshots/src/main/java/org/elasticsearch/xpack/searchablesnapshots/store/SearchableSnapshotDirectory.java index a3d233139bb7e..a3a172ccfdb4d 100644 --- a/x-pack/plugin/searchable-snapshots/src/main/java/org/elasticsearch/xpack/searchablesnapshots/store/SearchableSnapshotDirectory.java +++ b/x-pack/plugin/searchable-snapshots/src/main/java/org/elasticsearch/xpack/searchablesnapshots/store/SearchableSnapshotDirectory.java @@ -506,13 +506,32 @@ private void prewarmCache(ActionListener listener, Supplier cance final AtomicLong prefetchedBytes = new AtomicLong(0L); try (var fileListener = new RefCountingListener(ActionListener.runBefore(completionListener.acquire().map(v -> { + // we don't support files to be reported as partially recovered from disk and partially from the blob store, but + // this is something that can happen for fully mounted searchable snapshots. It is possible that prewarming + // prefetched nothing if a concurrent search was executing (and cached the data) or if the data were fetched from + // the blob cache system index. if (prefetchedBytes.get() == 0L) { recoveryState.markIndexFileAsReused(file.physicalName()); } else { - recoveryState.getIndex().addRecoveredFromSnapshotBytesToFile(file.physicalName(), prefetchedBytes.get()); + recoveryState.getIndex().addRecoveredFromSnapshotBytesToFile(file.physicalName(), file.length()); } return v; }), () -> IOUtils.closeWhileHandlingException(input)))) { + + if (input instanceof CachedBlobContainerIndexInput cachedIndexInput) { + if (cachedIndexInput.getPersistentCacheInitialLength() == file.length()) { + logger.trace( + () -> format( + "%s file [%s] is already available in cache (%d bytes)", + shardId, + file.physicalName(), + file.length() + ) + ); + continue; + } + } + for (int p = 0; p < file.numberOfParts(); p++) { final int part = p; prewarmTaskRunner.enqueueTask(fileListener.acquire().map(releasable -> { diff --git a/x-pack/plugin/searchable-snapshots/src/main/java/org/elasticsearch/xpack/searchablesnapshots/store/input/CachedBlobContainerIndexInput.java b/x-pack/plugin/searchable-snapshots/src/main/java/org/elasticsearch/xpack/searchablesnapshots/store/input/CachedBlobContainerIndexInput.java index 1be102ced288d..516b8f14735e1 100644 --- a/x-pack/plugin/searchable-snapshots/src/main/java/org/elasticsearch/xpack/searchablesnapshots/store/input/CachedBlobContainerIndexInput.java +++ b/x-pack/plugin/searchable-snapshots/src/main/java/org/elasticsearch/xpack/searchablesnapshots/store/input/CachedBlobContainerIndexInput.java @@ -127,6 +127,13 @@ protected void readWithoutBlobCache(ByteBuffer b) throws Exception { assert bytesRead == length : bytesRead + " vs " + length; } + /** + * @return Returns the number of bytes already cached for the file in the cold persistent cache + */ + public long getPersistentCacheInitialLength() throws Exception { + return cacheFileReference.get().getInitialLength(); + } + /** * Prefetches a complete part and writes it in cache. This method is used to prewarm the cache. *