Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use a dedicated thread pool for searchable snapshot cache prewarming #59313

Merged
merged 3 commits into from Jul 15, 2020

Conversation

tlrx
Copy link
Member

@tlrx tlrx commented Jul 9, 2020

Since #58728 writing operations on searchable snapshot directory cache files are executed in an asynchronous manner using a dedicated thread pool. The thread pool used is searchable_snapshots which has been created to execute prewarming tasks.

Reusing the same thread pool wasn't a good idea as it can lead to deadlock situations. One of these situation arose in a test failure where the thread pool was full of prewarming tasks, all waiting for a cache file to be accessible, while the cache file was being evicted by the cache service. But such an eviction can only be processed when all read/write operations on the cache file are completed and in this case the deadlock occurred because the cache file was actively being read by a concurrent search which also won the privilege to write the range of bytes in cache... and this writing operation could never have been completed
because of the prewarming tasks making no progress and filling up the thread pool.

This pull request renames the searchable_snapshots thread pool to searchable_snapshots_cache_fetch_async. Assertions are added to assert that cache writes are executed using this thread pool and to assert that read on cached index inputs are executed using a different thread pool to avoid potential deadlock situations.

This pull request also adds a searchable_snapshots_cache_prewarming that is used to execute prewarming tasks. It also converts the existing cache prewarming test into a more complte integration test that creates multiple searchable snapshot indices concurrently with randomized thread pool sizes, and verifies that all files have been correctly prewarmed.

After this pull request other improvements could be done:

  • limit the number of concurrent shards that are actively prewarming
  • make the prewarming of each file part fully asynchronous

@tlrx tlrx added :Distributed/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs v7.8.2 v7.9.0 v8.0.0 labels Jul 9, 2020
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-distributed (:Distributed/Snapshot/Restore)

@elasticmachine elasticmachine added the Team:Distributed Meta label for distributed team label Jul 9, 2020
@tlrx tlrx added >enhancement and removed Team:Distributed Meta label for distributed team labels Jul 9, 2020
@tlrx tlrx requested review from DaveCTurner and ywelsch July 9, 2020 16:31
Copy link
Contributor

@DaveCTurner DaveCTurner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@tlrx tlrx merged commit 3ab0445 into elastic:master Jul 15, 2020
@tlrx tlrx deleted the use-dedicated-thread-pool-for-prewarming branch July 15, 2020 07:58
@tlrx
Copy link
Member Author

tlrx commented Jul 15, 2020

Thanks David!

tlrx added a commit to tlrx/elasticsearch that referenced this pull request Jul 15, 2020
…lastic#59313)

Since elastic#58728 writing operations on searchable snapshot directory cache files
are executed in an asynchronous manner using a dedicated thread pool. The
thread pool used is searchable_snapshots which has been created to execute
prewarming tasks.

Reusing the same thread pool wasn't a good idea as it can lead to deadlock
situations. One of these situation arose in a test failure where the thread pool
was full of prewarming tasks, all waiting for a cache file to be accessible, while
the cache file was being evicted by the cache service. But such an eviction
can only be processed when all read/write operations on the cache file are
completed and in this case the deadlock occurred because the cache file was
actively being read by a concurrent search which also won the privilege to
write the range of bytes in cache... and this writing operation could never have
 been completed because of the prewarming tasks making no progress and
filling up the thread pool.

This commit renames the searchable_snapshots thread pool to
searchable_snapshots_cache_fetch_async. Assertions are added to assert
that cache writes are executed using this thread pool and to assert that read
on cached index inputs are executed using a different thread pool to avoid
potential deadlock situations.

This commit also adds a searchable_snapshots_cache_prewarming that is
used to execute prewarming tasks. It also converts the existing cache prewarming
test into a more complte integration test that creates multiple searchable
snapshot indices concurrently with randomized thread pool sizes, and verifies
that all files have been correctly prewarmed.
tlrx added a commit that referenced this pull request Jul 15, 2020
…59313) (#59590)

Since #58728 writing operations on searchable snapshot directory cache files
are executed in an asynchronous manner using a dedicated thread pool. The
thread pool used is searchable_snapshots which has been created to execute
prewarming tasks.

Reusing the same thread pool wasn't a good idea as it can lead to deadlock
situations. One of these situation arose in a test failure where the thread pool
was full of prewarming tasks, all waiting for a cache file to be accessible, while
the cache file was being evicted by the cache service. But such an eviction
can only be processed when all read/write operations on the cache file are
completed and in this case the deadlock occurred because the cache file was
actively being read by a concurrent search which also won the privilege to
write the range of bytes in cache... and this writing operation could never have
 been completed because of the prewarming tasks making no progress and
filling up the thread pool.

This commit renames the searchable_snapshots thread pool to
searchable_snapshots_cache_fetch_async. Assertions are added to assert
that cache writes are executed using this thread pool and to assert that read
on cached index inputs are executed using a different thread pool to avoid
potential deadlock situations.

This commit also adds a searchable_snapshots_cache_prewarming that is
used to execute prewarming tasks. It also converts the existing cache prewarming
test into a more complte integration test that creates multiple searchable
snapshot indices concurrently with randomized thread pool sizes, and verifies
that all files have been correctly prewarmed.
tlrx added a commit that referenced this pull request Jul 15, 2020
…59313) (#59595)

Since #58728 writing operations on searchable snapshot directory cache files
are executed in an asynchronous manner using a dedicated thread pool. The
thread pool used is searchable_snapshots which has been created to execute
prewarming tasks.

Reusing the same thread pool wasn't a good idea as it can lead to deadlock
situations. One of these situation arose in a test failure where the thread pool
was full of prewarming tasks, all waiting for a cache file to be accessible, while
the cache file was being evicted by the cache service. But such an eviction
can only be processed when all read/write operations on the cache file are
completed and in this case the deadlock occurred because the cache file was
actively being read by a concurrent search which also won the privilege to
write the range of bytes in cache... and this writing operation could never have
 been completed because of the prewarming tasks making no progress and
filling up the thread pool.

This commit renames the searchable_snapshots thread pool to
searchable_snapshots_cache_fetch_async. Assertions are added to assert
that cache writes are executed using this thread pool and to assert that read
on cached index inputs are executed using a different thread pool to avoid
potential deadlock situations.

This commit also adds a searchable_snapshots_cache_prewarming that is
used to execute prewarming tasks. It also converts the existing cache prewarming
test into a more complte integration test that creates multiple searchable
snapshot indices concurrently with randomized thread pool sizes, and verifies
that all files have been correctly prewarmed.
malpani pushed a commit to malpani/elasticsearch that referenced this pull request Jul 17, 2020
…lastic#59313)

Since elastic#58728 writing operations on searchable snapshot directory cache files 
are executed in an asynchronous manner using a dedicated thread pool. The 
thread pool used is searchable_snapshots which has been created to execute 
prewarming tasks.

Reusing the same thread pool wasn't a good idea as it can lead to deadlock 
situations. One of these situation arose in a test failure where the thread pool 
was full of prewarming tasks, all waiting for a cache file to be accessible, while 
the cache file was being evicted by the cache service. But such an eviction 
can only be processed when all read/write operations on the cache file are 
completed and in this case the deadlock occurred because the cache file was 
actively being read by a concurrent search which also won the privilege to 
write the range of bytes in cache... and this writing operation could never have
 been completed because of the prewarming tasks making no progress and 
filling up the thread pool.

This commit renames the searchable_snapshots thread pool to 
searchable_snapshots_cache_fetch_async. Assertions are added to assert 
that cache writes are executed using this thread pool and to assert that read 
on cached index inputs are executed using a different thread pool to avoid 
potential deadlock situations.

This commit also adds a searchable_snapshots_cache_prewarming that is 
used to execute prewarming tasks. It also converts the existing cache prewarming 
test into a more complte integration test that creates multiple searchable 
snapshot indices concurrently with randomized thread pool sizes, and verifies 
that all files have been correctly prewarmed.
henningandersen added a commit to henningandersen/elasticsearch that referenced this pull request Oct 20, 2020
The number of prewarming workers to spawn was taken from the wrong
thread pool info, fixed to size number of workers after the prewarming
thread pool.

Relates elastic#59313
henningandersen added a commit to henningandersen/elasticsearch that referenced this pull request Oct 20, 2020
The number of prewarming workers to spawn was taken from the wrong
thread pool info, fixed to size number of workers after the prewarming
thread pool.

Relates elastic#59313
henningandersen added a commit that referenced this pull request Oct 22, 2020
The number of prewarming workers to spawn was taken from the wrong
thread pool info, fixed to size number of workers after the prewarming
thread pool.

Relates #59313
henningandersen added a commit that referenced this pull request Oct 22, 2020
The number of prewarming workers to spawn was taken from the wrong
thread pool info, fixed to size number of workers after the prewarming
thread pool.

Relates #59313
@jakelandis jakelandis removed the v8.0.0 label Jul 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants