Trigger explicit loading of blob container and snapshot in the pre-recovery hook#54729
Merged
tlrx merged 3 commits intoelastic:feature/searchable-snapshotsfrom Apr 6, 2020
Merged
Conversation
Collaborator
|
Pinging @elastic/es-distributed (:Distributed/Snapshot/Restore) |
Member
Author
|
ML related failures + Docker build failure @elasticmachine run elasticsearch-ci/2 |
Member
Author
|
Running tests again after an unrelated failure |
DaveCTurner
reviewed
Apr 6, 2020
Member
DaveCTurner
left a comment
There was a problem hiding this comment.
Looks good, I left a few small suggestions.
...hable-snapshots/src/main/java/org/elasticsearch/index/store/SearchableSnapshotDirectory.java
Outdated
Show resolved
Hide resolved
|
|
||
| private synchronized boolean invariant() { | ||
| assert loaded ^ snapshot == null; | ||
| assert loaded ^ blobContainer == null; |
Member
There was a problem hiding this comment.
Technically ok, but I think a logical XOR is clearer than a bitwise XOR:
Suggested change
| assert loaded ^ blobContainer == null; | |
| assert loaded != (blobContainer == null); |
...hable-snapshots/src/main/java/org/elasticsearch/index/store/SearchableSnapshotDirectory.java
Show resolved
Hide resolved
...hable-snapshots/src/main/java/org/elasticsearch/index/store/SearchableSnapshotDirectory.java
Show resolved
Hide resolved
| final BlobContainer blobContainer = this.blobContainer.get(); | ||
| assert blobContainer != null; | ||
| final BlobContainer blobContainer = this.blobContainer; | ||
| assert invariant(); |
Member
There was a problem hiding this comment.
I don't think we need to assert this here since we're not changing any state?
| final BlobStoreIndexShardSnapshot snapshot = this.snapshot.get(); | ||
| assert snapshot != null; | ||
| final BlobStoreIndexShardSnapshot snapshot = this.snapshot; | ||
| assert invariant(); |
Member
There was a problem hiding this comment.
I don't think we need to assert this here since we're not changing any state?
| } | ||
|
|
||
| private List<BlobStoreIndexShardSnapshot.FileInfo> files() { | ||
| final BlobStoreIndexShardSnapshot snapshot = snapshot(); |
Member
There was a problem hiding this comment.
Relates my earlier comment - let's return an empty list if loaded == false, then we can be sure that snapshot() will return non-null.
Member
Author
There was a problem hiding this comment.
Sure. I must admit I find this code confusing today :/
DaveCTurner
approved these changes
Apr 6, 2020
...hable-snapshots/src/main/java/org/elasticsearch/index/store/SearchableSnapshotDirectory.java
Show resolved
Hide resolved
Member
Author
|
Thanks David! |
DaveCTurner
added a commit
to DaveCTurner/elasticsearch
that referenced
this pull request
Apr 7, 2020
Today the shards of searchable snapshots are allocated with a naive `ExistingShardsAllocator` which selects the first valid node for each shard. Thanks to elastic#54729 we can now allow these shards to fall through to the balanced shards allocator so that they are allocated in a more balanced fashion. Relates elastic#50999
DaveCTurner
added a commit
that referenced
this pull request
Apr 8, 2020
Today the shards of searchable snapshots are allocated with a naive `ExistingShardsAllocator` which selects the first valid node for each shard. Thanks to #54729 we can now allow these shards to fall through to the balanced shards allocator so that they are allocated in a more balanced fashion. Relates #50999
DaveCTurner
added a commit
that referenced
this pull request
Apr 8, 2020
Today the shards of searchable snapshots are allocated with a naive `ExistingShardsAllocator` which selects the first valid node for each shard. Thanks to #54729 we can now allow these shards to fall through to the balanced shards allocator so that they are allocated in a more balanced fashion. Relates #50999
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
In #53961 we defer the construction of Searchable Snapshot Directory's
BlobContainerand aBlobStoreIndexShardSnapshotinstances so that these objects are created when they are first accessed, which we expect to be during the recovery process. At the same time, assertions were added to ensure that the construction is effectively executed under a generic or snapshot thread.Sadly, this is not always the case because there is always a short window of time between the creation of the
IndexShardand the time the objects are created during the recovery process. It is also possible that other components of Elasticsearch trigger the creation of the blob container and snapshot.We identified the following places:
This commit changes the way
BlobContainerand aBlobStoreIndexShardSnapshotinstances are created so that it does not happen on first access anymore but the objects are now created using a specificloadSnapshot()method.This method is explicitly called during the
pre-recoveryphase.Until this method is called, the
SearchableSnapshotDirectoryacts as if it was empty: the listAll() method returns an empty array. Having this behavior allows the identified access points to not fail and not trigger the snapshot loading before we explicitly load it in the pre-recovery hook.