Make searchable snapshots cache persistent #65725

tlrx · 2020-12-02T10:46:29Z

The searchable snapshots cache implemented in 7.10 is not persisted across node restarts, forcing data nodes to download files from the snapshot repository again once the node is restarted.

This pull request introduces a new Lucene index that is used to store information about cache files. The information about cache files are periodically updated and committed in this index as part of the cache synchronization task added in #64696. When the data node starts the Lucene index is used to load in memory the cache files information; these information are then used to repopulate the searchable snapshots cache with the cache files that exist on disk.

Since data nodes can have one or more data paths, this change introduces a Lucene index per data path. Information about cache files are updated in the Lucene index located on the same data path of the cache files.

This pull request is lacking some tests and maybe documentation. There are also a test failure that I'm still tracking, but I'd like to start reviews so that we can move forward.

elasticmachine · 2020-12-02T10:47:12Z

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

tlrx · 2020-12-02T13:23:18Z

@original-brownbear @henningandersen this is ready for a first round of reviews. I suspect that a situation is not well handled as one of our main test keeps failing, which I'm investigating, but we can start grabing feedback on the main change.

henningandersen

I did an initial read of the production code, looks good.

henningandersen · 2020-12-02T14:33:39Z

...apshots/src/main/java/org/elasticsearch/xpack/searchablesnapshots/cache/PersistentCache.java

+ final NodeEnvironment.NodePath nodePath = writer.nodePath();
+ logger.debug("loading persistent cache on data path [{}]", nodePath);
+
+ for (String indexUUID : nodeEnvironment.availableIndexFoldersForPath(writer.nodePath())) {


nit:

Suggested change

for (String indexUUID : nodeEnvironment.availableIndexFoldersForPath(writer.nodePath())) {

for (String indexUUID : nodeEnvironment.availableIndexFoldersForPath(nodePath)) {

Thanks, I pushed 9bef60d

henningandersen · 2020-12-02T15:12:29Z

...apshots/src/main/java/org/elasticsearch/xpack/searchablesnapshots/cache/PersistentCache.java

+ final Map<String, Document> documents = new HashMap<>();
+ try (IndexReader indexReader = DirectoryReader.open(directory)) {
+ for (LeafReaderContext leafReaderContext : indexReader.leaves()) {
+ final LeafReader leafReader = leafReaderContext.reader();
+ final Bits liveDocs = leafReader.getLiveDocs();
+ for (int i = 0; i < leafReader.maxDoc(); i++) {
+ if (liveDocs == null || liveDocs.get(i)) {
+ final Document document = leafReader.document(i);
+ documents.put(getValue(document, CACHE_ID_FIELD), document);
+ }
+ }
+ }
+ } catch (IndexNotFoundException e) {
+ logger.debug("persistent cache index does not exist yet", e);
+ }


Would it be possible to extract this to a separate method and pass the map directly to the CacheFileVisitor? It feels a bit backwards to pass it through the CacheIndexWriter, but there might be a valid reason for it?

No valid reason, I agree that seems a bit backward. I pushed 5dcd53a

henningandersen · 2020-12-03T09:34:52Z

...apshots/src/main/java/org/elasticsearch/xpack/searchablesnapshots/cache/PersistentCache.java

+ document.add(new StringField(INDEX_NAME_FIELD, indexId.getName(), Field.Store.YES));
+ document.add(new StringField(INDEX_ID_FIELD, indexId.getId(), Field.Store.YES));


Are these different from the shard index name/id below?

Yes, those ones are referring to the index in the snapshot while shard's index name/id are referring to the index assigned to the data node.

Maybe one point question here:

Should we simplify CacheKey maybe before we move to persisting the format? like we're going to here? There is no need to store the indexId IMO. The following information is enough to uniquely identify a shard in a given snapshot:

snapshot_uuid

index name

shard id (int)

All the other information like index uuid and and IndexId are just noise. Having redundant index_uuid in here also technically allows for having the same file in the cache twice for different mounts of an index? (not that it makes sense to have those but it complicates the logic for figuring out what is already in the cache for a given index in the allocation work)

We can indeed simplify the CacheKey and removes snapshot's name and snapshot's index uuid. We need to keep the shard's IndexId because as of today cache files are located within a specified shard data path, so they belong to a specific shard within the cluster and need to follow this shard lifecycle.

because as of today cache files are located within a specified shard data path, so they belong to a specific shard within the cluster and need to follow this shard lifecycle

Makes sense thanks :) shall we drop that here or is BwC trivial if we chose to simplify this later (it looks trivial actually and we can just ignore the extra fields in the documents in a follow-up ... but maybe I'm missing something)?

You're welcome :) I think that the change will be trivial as you said. Actually I think I can do it as a follow up but because this is spread in many classes doing it now will introduce a lot of noise (and potential merge conflicts).

tlrx · 2020-12-11T12:28:50Z

@henningandersen @original-brownbear I merged the latest changes from master in this pull request, most notably :

Change index removal reason when IndicesService is stopping (Change index removal reason when IndicesService is stopping #65816)
Introduce a mechanism to notify plugin before an index/shard folder is going to be deleted from disk (Introduce a mechanism to notify plugin before an index/shard folder is going to be deleted from disk #65926)
Be more specific when clearing cache for searchable snapshots shards (Be more specific when clearing cache for searchable snapshots shards #66003)
SearchableSnapshotDirectory should not evict cache files when closed (SearchableSnapshotDirectory should not evict cache files when closed #66173)

I also removed the previously muted test that should succeed now thanks to all of these changes. I plan to add few more IT but I can take this as follow ups because this pull request is already large.

This is ready for reviews.

original-brownbear

A few initial comments (the one on the format of the Lucene documents/keys is why I'm snap commenting here :) since I had that question working on the allocation as well)

...apshots/src/main/java/org/elasticsearch/xpack/searchablesnapshots/cache/PersistentCache.java

original-brownbear · 2020-12-14T07:38:12Z

...apshots/src/main/java/org/elasticsearch/xpack/searchablesnapshots/cache/PersistentCache.java

+
+ if (Files.isDirectory(shardCachePath)) {
+ logger.trace("found snapshot cache dir at [{}], loading cache files from disk and index", shardCachePath);
+ Files.walkFileTree(shardCachePath, new CacheFileVisitor(cacheService, writer, documents));


NIT: This visitor may be easier to read if it was just inlined and one doesn't have to jump around to the other class to figure out what's going on (also saves a few lines for the constructor)?

logger.trace("found snapshot cache dir at [{}], loading cache files from disk and index", shardCachePath); Files.walkFileTree(shardCachePath, new SimpleFileVisitor<>() { @Override public FileVisitResult visitFile(Path file, BasicFileAttributes attrs) { try { final String id = buildId(file); final Document cacheDocument = documents.get(id); if (cacheDocument != null) { logger.trace("indexing cache file with id [{}] in persistent cache index", id); writer.updateCacheFile(id, cacheDocument); final CacheKey cacheKey = buildCacheKey(cacheDocument); logger.trace("adding cache file with [id={}, cache key={}]", id, cacheKey); final long fileLength = getFileLength(cacheDocument); cacheService.put(cacheKey, fileLength, file.getParent(), id, buildCacheFileRanges(cacheDocument)); } else { logger.trace("deleting cache file [{}] (does not exist in persistent cache index)", file); Files.delete(file); } } catch (Exception e) { throw ExceptionsHelper.convertToRuntime(e); } return FileVisitResult.CONTINUE; } }); }

Sure, I pushed ac6050f

original-brownbear · 2020-12-14T07:43:17Z

...apshots/src/main/java/org/elasticsearch/xpack/searchablesnapshots/cache/PersistentCache.java

+ ensureOpen();
+ try {
+ for (CacheIndexWriter writer : writers) {
+ writer.prepareCommit();


Do we actually need two loops for preparing and then committing? We do that for the CS persistence so we can throw IOError and stop everything if a commit fails but for the cache service it seems like we could just commit in a loop and ahve that deal with prepareCommit? That would also make exceptions less noisy because if you prepare a commit on one writer and then fail committing another writer and thus call close close the first writer will throw on closing because it has a pending commit as well.

That's a good point, thanks for catching this. Commiting in a single loop makes it less error prone as you pointed. I pushed ac6050f

original-brownbear · 2020-12-14T07:50:02Z

...apshots/src/main/java/org/elasticsearch/xpack/searchablesnapshots/cache/PersistentCache.java

+ document.add(new StringField(INDEX_NAME_FIELD, indexId.getName(), Field.Store.YES));
+ document.add(new StringField(INDEX_ID_FIELD, indexId.getId(), Field.Store.YES));


Maybe one point question here:

Should we simplify CacheKey maybe before we move to persisting the format? like we're going to here? There is no need to store the indexId IMO. The following information is enough to uniquely identify a shard in a given snapshot:

snapshot_uuid

index name

shard id (int)

All the other information like index uuid and and IndexId are just noise. Having redundant index_uuid in here also technically allows for having the same file in the cache twice for different mounts of an index? (not that it makes sense to have those but it complicates the logic for figuring out what is already in the cache for a given index in the allocation work)

original-brownbear

LGTM (outside of the points raised by Henning. Read through it fully now and couldn't find anything to add so all good from my end, thanks!

tlrx · 2020-12-14T13:32:14Z

Thanks @henningandersen and @original-brownbear ! I've applied some changes after your feedback, let me know what you think.

henningandersen

LGTM.

tlrx · 2020-12-14T14:26:03Z

Thanks a lot Henning and Armin!

The searchable snapshots cache implemented in 7.10 is not persisted across node restarts, forcing data nodes to download files from the snapshot repository again once the node is restarted. This commit introduces a new Lucene index that is used to store information about cache files. The information about cache files are periodically updated and committed in this index as part of the cache synchronization task added in elastic#64696. When the data node starts the Lucene index is used to load in memory the cache files information; these information are then used to repopulate the searchable snapshots cache with the cache files that exist on disk. Since data nodes can have one or more data paths, this change introduces a Lucene index per data path. Information about cache files are updated in the Lucene index located on the same data path of the cache files.

The searchable snapshots cache implemented in 7.10 is not persisted across node restarts, forcing data nodes to download files from the snapshot repository again once the node is restarted. This commit introduces a new Lucene index that is used to store information about cache files. The information about cache files are periodically updated and committed in this index as part of the cache synchronization task added in #64696. When the data node starts the Lucene index is used to load in memory the cache files information; these information are then used to repopulate the searchable snapshots cache with the cache files that exist on disk. Since data nodes can have one or more data paths, this change introduces a Lucene index per data path. Information about cache files are updated in the Lucene index located on the same data path of the cache files. Backport of #65725 for 7.11

This commit simplifies the CacheKey object as suggested in #65725.

This commit simplifies the CacheKey object as suggested in elastic#65725.

This commit simplifies the CacheKey object as suggested in #65725. Backport of #66263 for 7.11

Add persistent cache

102d9aa

tlrx added :Distributed/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs >enhancement v7.11.0 v8.0.0 labels Dec 2, 2020

elasticmachine added the Team:Distributed Meta label for distributed team label Dec 2, 2020

tlrx added 4 commits December 2, 2020 12:31

indexed cache file path should be relative

6ff96eb

mute test + remove suppress filesystems

4b697a8

remove leftovers

039e8ea

Merge branch 'master' into add-persistent-cache

a864370

tlrx requested review from original-brownbear and henningandersen December 2, 2020 13:20

henningandersen reviewed Dec 3, 2020

View reviewed changes

tlrx added 3 commits December 3, 2020 12:02

nodePath

9bef60d

extract map

5dcd53a

Merge branch 'master' into add-persistent-cache

3364d44

tlrx requested a review from henningandersen December 3, 2020 13:00

tlrx added 7 commits December 4, 2020 17:02

Also match persistent cache index docs with cache files

381188f

Merge branch 'master' into add-persistent-cache

685baac

add comment + IndexRemovalReason.NO_LONGER_ASSIGNED

f6ecc3a

Merge branch 'master' into add-persistent-cache

c36996b

Merge branch 'master' into add-persistent-cache

5b2d653

remove after merge leftovers

8c12a96

more spotless fixes

255853d

original-brownbear reviewed Dec 14, 2020

View reviewed changes

tlrx added 2 commits December 14, 2020 09:14

Merge branch 'master' into add-persistent-cache

9e9ba7c

feedback

ac6050f

tlrx added 4 commits December 14, 2020 12:41

assert IOE

5aa545a

local var buffer

2940d20

should persist

eab0a0c

Fix tests

9f041f6

original-brownbear approved these changes Dec 14, 2020

View reviewed changes

tlrx added 2 commits December 14, 2020 14:27

test

5ca99d2

Merge branch 'master' into add-persistent-cache

3c9a8b1

tlrx requested a review from henningandersen December 14, 2020 13:32

henningandersen approved these changes Dec 14, 2020

View reviewed changes

tlrx merged commit 672972c into elastic:master Dec 14, 2020

tlrx deleted the add-persistent-cache branch December 14, 2020 14:25

tlrx added the backport pending label Dec 14, 2020

tlrx mentioned this pull request Dec 14, 2020

Simplify searchable snapshot CacheKey #66263

Merged

tlrx mentioned this pull request Dec 14, 2020

Make searchable snapshots cache persistent #66275

Merged

tlrx removed the backport pending label Dec 14, 2020

tlrx added a commit that referenced this pull request Dec 14, 2020

Simplify searchable snapshot CacheKey (#66263)

de495e0

This commit simplifies the CacheKey object as suggested in #65725.

tlrx added a commit to tlrx/elasticsearch that referenced this pull request Dec 14, 2020

Simplify searchable snapshot CacheKey (elastic#66263)

aaba9e2

This commit simplifies the CacheKey object as suggested in elastic#65725.

tlrx mentioned this pull request Dec 14, 2020

Simplify searchable snapshot CacheKey #66283

Merged

tlrx added a commit that referenced this pull request Dec 14, 2020

Simplify searchable snapshot CacheKey (#66263) (#66283)

3c5d60e

This commit simplifies the CacheKey object as suggested in #65725. Backport of #66263 for 7.11

tlrx mentioned this pull request Dec 14, 2020

Add searchable snapshot cache folder to NodeEnvironment #66297

Merged

tlrx added the release highlight label Jan 6, 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

Make searchable snapshots cache persistent #65725

Make searchable snapshots cache persistent #65725

tlrx commented Dec 2, 2020

elasticmachine commented Dec 2, 2020

tlrx commented Dec 2, 2020 •

edited

Loading

henningandersen left a comment

henningandersen Dec 2, 2020

tlrx Dec 3, 2020

henningandersen Dec 2, 2020

tlrx Dec 3, 2020

henningandersen Dec 3, 2020

tlrx Dec 3, 2020

original-brownbear Dec 14, 2020

tlrx Dec 14, 2020

original-brownbear Dec 14, 2020

tlrx Dec 14, 2020

tlrx commented Dec 11, 2020

original-brownbear left a comment

original-brownbear Dec 14, 2020

tlrx Dec 14, 2020

original-brownbear Dec 14, 2020

tlrx Dec 14, 2020

original-brownbear Dec 14, 2020

original-brownbear left a comment

tlrx commented Dec 14, 2020

henningandersen left a comment

tlrx commented Dec 14, 2020

	for (String indexUUID : nodeEnvironment.availableIndexFoldersForPath(writer.nodePath())) {
	for (String indexUUID : nodeEnvironment.availableIndexFoldersForPath(nodePath)) {

		document.add(new StringField(INDEX_NAME_FIELD, indexId.getName(), Field.Store.YES));
		document.add(new StringField(INDEX_ID_FIELD, indexId.getId(), Field.Store.YES));

Make searchable snapshots cache persistent #65725

Make searchable snapshots cache persistent #65725

Conversation

tlrx commented Dec 2, 2020

elasticmachine commented Dec 2, 2020

tlrx commented Dec 2, 2020 • edited Loading

henningandersen 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tlrx commented Dec 11, 2020

original-brownbear 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

original-brownbear left a comment

Choose a reason for hiding this comment

tlrx commented Dec 14, 2020

henningandersen left a comment

Choose a reason for hiding this comment

tlrx commented Dec 14, 2020

tlrx commented Dec 2, 2020 •

edited

Loading