Always use CacheService for caching metadata blobs #70668

ywelsch · 2021-03-22T16:31:57Z

This PR unifies CachedBlobContainerIndexInput and FrozenIndexInput so that they share the same infrastructure for caching metadata blobs as well as header and footer ranges for data blobs. The idea is to always use CacheService for this, which does not evict the metadata, and which efficiently stores the information on disk (using sparse file support).

This also allows us to align writes in FrozenCacheService to 4KB block sizes in this PR, which addresses an issue when reusing regions from the shared cache, as writes that are not aligned on page cache boundaries causes the existing data (which we don't care about) to be loaded from disk, which comes with a dramatic performance penalty.

In a follow-up, we should also add a test / bootstrap check that ensures that sparse file support is always provided.

Closes #70728
Closes #70763

elasticmachine · 2021-03-23T09:38:11Z

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

tlrx

This change makes sense to me. I've done a first pass and left some comments.

tlrx · 2021-03-23T12:45:01Z

...e-snapshots/src/main/java/org/elasticsearch/index/store/cache/MetadataCachingIndexInput.java

+
+    @Override
+    protected void doReadInternal(ByteBuffer b) throws IOException {
+        final long position = getAbsolutePosition();


I wonder if we should compute the absolute and the relative positions only once here and pass them along as parameters to all other read methods which will be able to use whatever position they want but in a more explicit manner (ie relativePosition + b.remaining() for example).

I've addressed the other comments here, but I'm not yet sure how to go about this one. I've pushed aaa69e6 which removes the virtual file logic from FrozenCacheService and which simplifies things a bit further when it comes to position calculations (same for FrozenIndexInput as well as CachedBlobContainerIndexInput now).

Ok, let's keep it as it is. I'll give it a try.

...e-snapshots/src/main/java/org/elasticsearch/index/store/cache/MetadataCachingIndexInput.java

tlrx · 2021-03-23T13:01:38Z

...searchable-snapshots/src/main/java/org/elasticsearch/index/store/cache/FrozenIndexInput.java

@@ -83,13 +57,13 @@ public FrozenIndexInput(
            0L,
            0L,
            fileInfo.length(),
+            new CacheFileReference(directory, fileInfo.physicalName(), fileInfo.length()),


I'm a bit torn on the file's length that is passed here. It means that we'll create a potentially large file in the cache service (and cause potential evictions) while we know it will only be used to cache at most directory.getBlobCacheByteRange(name, fileInfo.length()).length() bytes. We could maybe affine this?

Note that CacheService is always supposed to be unbounded (and that's how it should be, the goal is to remove the undocumented setting to even make this configurable, and remove the complexity around that)

Right, I forgot about that point. Also my suggestion was pretty bad since the file length here passed to the sparse file tracker and that would just break things. Sorry for the noise.

ywelsch · 2021-03-23T15:50:09Z

@elasticmachine run elasticsearch-ci/2 (timeout downloading stuff from the internet)

Relates to elastic#70763 and can be unmuted when elastic#70668 is unmuted

Relates to #70763 and can be unmuted when #70668 is unmuted

…lastic#70773)" This reverts commit 3cf2dd5.

tlrx

LGTM - I've left only minor things along the review, feel free to apply or ignore

tlrx · 2021-03-24T08:26:05Z

...ava/org/elasticsearch/xpack/searchablesnapshots/SearchableSnapshotsPrewarmingIntegTests.java

+        TrackingRepositoryPlugin tracker = null;
+        for (RepositoryPlugin plugin : getInstanceFromNode(PluginsService.class).filterPlugins(RepositoryPlugin.class)) {
+            if (plugin instanceof TrackingRepositoryPlugin) {
+                tracker = ((TrackingRepositoryPlugin) plugin);


nit: return here directly

tlrx · 2021-03-24T08:27:32Z

...org/elasticsearch/xpack/searchablesnapshots/SearchableSnapshotsUuidValidationIntegTests.java

@@ -33,6 +34,7 @@
 import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertAcked;
 import static org.hamcrest.Matchers.containsString;

+@ESIntegTestCase.ClusterScope(scope = ESIntegTestCase.Scope.TEST)


There is a single test in this class, why changing the scope to TEST?

It's because when you run the test with tests.iters=x, it will reuse the same node, and that breaks the test (RestoreBlockingActionFilter.executed and unblocked are not reset)

tlrx · 2021-03-24T08:36:41Z

...searchable-snapshots/src/main/java/org/elasticsearch/index/store/cache/FrozenIndexInput.java

@@ -83,13 +57,13 @@ public FrozenIndexInput(
            0L,
            0L,
            fileInfo.length(),
+            new CacheFileReference(directory, fileInfo.physicalName(), fileInfo.length()),


Right, I forgot about that point. Also my suggestion was pretty bad since the file length here passed to the sparse file tracker and that would just break things. Sorry for the noise.

...searchable-snapshots/src/main/java/org/elasticsearch/index/store/cache/FrozenIndexInput.java

tlrx · 2021-03-24T08:52:26Z

...searchable-snapshots/src/main/java/org/elasticsearch/index/store/cache/FrozenIndexInput.java

        long bytesWritten = positionalWrite(fc, fileChannelPos + bytesCopied, buf);
-        bytesCopied += bytesWritten;
+        bytesCopied += (bytesWritten - adjustment); // adjust to not break RangeFileTracker


Maybe the stats should show the exact bytes copied (ie we can confirm the 4K alignment) but progress updater got updated with the adjusted bytes copied?

I will defer that to a follow-up as it will break a lot of tests (again)

tlrx · 2021-03-24T08:55:02Z

...e-snapshots/src/main/java/org/elasticsearch/index/store/cache/MetadataCachingIndexInput.java

+
+    @Override
+    protected void doReadInternal(ByteBuffer b) throws IOException {
+        final long position = getAbsolutePosition();


Ok, let's keep it as it is. I'll give it a try.

This PR unifies CachedBlobContainerIndexInput and FrozenIndexInput so that they share the same infrastructure for caching metadata blobs as well as header and footer ranges for data blobs. The idea is to always use CacheService for this, which does not evict the metadata, and which efficiently stores the information on disk (using sparse file support). This also allows us to align writes in FrozenCacheService to 4KB block sizes in this PR, which addresses an issue when reusing regions from the shared cache, as writes that are not aligned on page cache boundaries causes the existing data (which we don't care about) to be loaded from disk, which comes with a dramatic performance penalty. Closes elastic#70728 Closes elastic#70763

This PR unifies CachedBlobContainerIndexInput and FrozenIndexInput so that they share the same infrastructure for caching metadata blobs as well as header and footer ranges for data blobs. The idea is to always use CacheService for this, which does not evict the metadata, and which efficiently stores the information on disk (using sparse file support). This also allows us to align writes in FrozenCacheService to 4KB block sizes in this PR, which addresses an issue when reusing regions from the shared cache, as writes that are not aligned on page cache boundaries causes the existing data (which we don't care about) to be loaded from disk, which comes with a dramatic performance penalty. Closes #70728 Closes #70763

…lastic#70795) This PR unifies CachedBlobContainerIndexInput and FrozenIndexInput so that they share the same infrastructure for caching metadata blobs as well as header and footer ranges for data blobs. The idea is to always use CacheService for this, which does not evict the metadata, and which efficiently stores the information on disk (using sparse file support). This also allows us to align writes in FrozenCacheService to 4KB block sizes in this PR, which addresses an issue when reusing regions from the shared cache, as writes that are not aligned on page cache boundaries causes the existing data (which we don't care about) to be loaded from disk, which comes with a dramatic performance penalty. Closes elastic#70728 Closes elastic#70763

ywelsch added 9 commits March 22, 2021 15:21

Add CFS index caching support for full_copy searchable snapshots

c5f22c7

Extract caching

4c7ba06

test randomization

e705699

fixes

912d73b

more fixes

6073500

rename

f4355bc

read from cache first

6dc889f

Merge remote-tracking branch 'elastic/master' into extract-caching

e49bd8d

cosmetics

ab7bb59

ywelsch added :Distributed Coordination/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs >enhancement v7.13.0 v8.0.0 labels Mar 23, 2021

ywelsch marked this pull request as ready for review March 23, 2021 09:38

elasticmachine added the Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination. label Mar 23, 2021

ywelsch requested a review from tlrx March 23, 2021 10:45

tlrx reviewed Mar 23, 2021

View reviewed changes

ywelsch added 2 commits March 23, 2021 16:16

review feedback

9d07c8d

No more virtual files

aaa69e6

This was referenced Mar 23, 2021

Failure in CachedBlobContainerIndexInputTests.testRandomReads #70763

Closed

SearchableSnapshotDirectoryStatsTests failures #70728

Closed

dakrone added a commit to dakrone/elasticsearch that referenced this pull request Mar 23, 2021

Mute AbstractSearchableSnapshotsTestCase and all sub tests

a2942c0

Relates to elastic#70763 and can be unmuted when elastic#70668 is unmuted

dakrone mentioned this pull request Mar 23, 2021

Mute AbstractSearchableSnapshotsTestCase and all sub tests #70773

Merged

dakrone added a commit that referenced this pull request Mar 23, 2021

Mute AbstractSearchableSnapshotsTestCase and all sub tests (#70773)

3cf2dd5

Relates to #70763 and can be unmuted when #70668 is unmuted

dakrone added a commit that referenced this pull request Mar 23, 2021

Mute AbstractSearchableSnapshotsTestCase and all sub tests (#70773)

a36d86d

Relates to #70763 and can be unmuted when #70668 is unmuted

ywelsch added 3 commits March 23, 2021 22:45

Merge remote-tracking branch 'elastic/master' into extract-caching

3e13639

Revert "Mute AbstractSearchableSnapshotsTestCase and all sub tests (e…

3d0fd78

…lastic#70773)" This reverts commit 3cf2dd5.

make tests compatible with running in loop

05278c6

ywelsch requested a review from tlrx March 23, 2021 22:45

generalize test :)

a6e96cc

tlrx approved these changes Mar 24, 2021

View reviewed changes

ywelsch added 2 commits March 24, 2021 10:06

feedback

3c670d6

not yet

5cd00fa

ywelsch merged commit 296ac1a into elastic:master Mar 24, 2021

ywelsch added the v7.12.1 label Mar 26, 2021

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Always use CacheService for caching metadata blobs #70668

Always use CacheService for caching metadata blobs #70668

ywelsch commented Mar 22, 2021 •

edited

Loading

elasticmachine commented Mar 23, 2021

tlrx left a comment

tlrx Mar 23, 2021

ywelsch Mar 23, 2021

tlrx Mar 24, 2021

tlrx Mar 23, 2021

ywelsch Mar 23, 2021

tlrx Mar 24, 2021

ywelsch commented Mar 23, 2021

tlrx left a comment

tlrx Mar 24, 2021

tlrx Mar 24, 2021

ywelsch Mar 24, 2021

tlrx Mar 24, 2021

tlrx Mar 24, 2021

ywelsch Mar 24, 2021

ywelsch Mar 24, 2021

tlrx Mar 24, 2021

Always use CacheService for caching metadata blobs #70668

Always use CacheService for caching metadata blobs #70668

Conversation

ywelsch commented Mar 22, 2021 • edited Loading

elasticmachine commented Mar 23, 2021

tlrx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ywelsch commented Mar 23, 2021

tlrx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ywelsch commented Mar 22, 2021 •

edited

Loading