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

pageserver: use adaptive concurrency in secondary layer downloads #7675

Merged
merged 4 commits into from
May 13, 2024

Conversation

jcsp
Copy link
Contributor

@jcsp jcsp commented May 9, 2024

Problem

Secondary downloads are a low priority task, and intentionally do not try to max out download speeds. This is almost always fine when they are used through the life of a tenant shard as a continuous "trickle" of background downloads.

However, there are sometimes circumstances where we would like to populate a secondary location as fast as we can, within the constraint that we don't want to impact the activity of attached tenants:

  • During node removal, where we will need to create replacements for secondary locations on the node being removed
  • After a shard split, we need new secondary locations for the new shards to populate before the shards can be migrated to their final location.

Summary of changes

  • Add an activity() function to the remote storage interface, enabling callers to query how busy the remote storage backend is
  • In the secondary download code, use a very modest amount of concurrency, driven by the remote storage's state: we only use concurrency if the remote storage semaphore is 75% free, and scale the amount of concurrency used within that range.

This is not a super clever form of prioritization, but it should accomplish the key goals:

  • Enable secondary downloads to happen faster when the system is idle
  • Make secondary downloads a much lower priority than attached tenants when the remote storage is busy.

Checklist before requesting a review

  • I have performed a self-review of my code.
  • If it is a core feature, I have added thorough tests.
  • Do we need to implement analytics? if so did you add the relevant metrics to the dashboard?
  • If this PR requires public announcement, mark it with /release-notes label and add several sentences in this section.

Checklist before merging

  • Do not forget to reformat commit message to not include the above checklist

@jcsp jcsp added c/storage/pageserver Component: storage: pageserver a/tech_debt Area: related to tech debt labels May 9, 2024
Copy link

github-actions bot commented May 9, 2024

3060 tests run: 2927 passed, 0 failed, 133 skipped (full report)


Code coverage* (full report)

  • functions: 31.4% (6334 of 20179 functions)
  • lines: 47.3% (47882 of 101230 lines)

* collected from Rust tests only


The comment gets automatically updated with the latest test results
539dcbf at 2024-05-13T17:48:46.323Z :recycle:

@jcsp jcsp force-pushed the jcsp/secondary-concurrency branch from 5fb008b to 4c2be5a Compare May 12, 2024 17:44
@jcsp jcsp marked this pull request as ready for review May 13, 2024 08:44
@jcsp jcsp requested a review from a team as a code owner May 13, 2024 08:44
@jcsp jcsp requested a review from arpad-m May 13, 2024 08:44
pageserver/src/tenant/secondary/downloader.rs Show resolved Hide resolved
pageserver/src/tenant/secondary/downloader.rs Outdated Show resolved Hide resolved
Co-authored-by: Arpad Müller <arpad-m@users.noreply.github.com>
@jcsp jcsp enabled auto-merge (squash) May 13, 2024 16:59
@jcsp jcsp merged commit 972470b into main May 13, 2024
50 checks passed
@jcsp jcsp deleted the jcsp/secondary-concurrency branch May 13, 2024 17:38
a-masterov pushed a commit that referenced this pull request May 20, 2024
)

## Problem

Secondary downloads are a low priority task, and intentionally do not
try to max out download speeds. This is almost always fine when they are
used through the life of a tenant shard as a continuous "trickle" of
background downloads.

However, there are sometimes circumstances where we would like to
populate a secondary location as fast as we can, within the constraint
that we don't want to impact the activity of attached tenants:
- During node removal, where we will need to create replacements for
secondary locations on the node being removed
- After a shard split, we need new secondary locations for the new
shards to populate before the shards can be migrated to their final
location.

## Summary of changes

- Add an activity() function to the remote storage interface, enabling
callers to query how busy the remote storage backend is
- In the secondary download code, use a very modest amount of
concurrency, driven by the remote storage's state: we only use
concurrency if the remote storage semaphore is 75% free, and scale the
amount of concurrency used within that range.

This is not a super clever form of prioritization, but it should
accomplish the key goals:
- Enable secondary downloads to happen faster when the system is idle
- Make secondary downloads a much lower priority than attached tenants
when the remote storage is busy.

---------

Co-authored-by: Arpad Müller <arpad-m@users.noreply.github.com>
jcsp added a commit that referenced this pull request May 23, 2024
…oads`

AKA Pull request #7675

This partially reverts commit 5f0fbdf.  We keep the part of that PR that
refactored download_layer into a function.
jcsp added a commit that referenced this pull request May 24, 2024
…oads`

AKA Pull request #7675

This partially reverts commit 5f0fbdf.  We keep the part of that PR that
refactored download_layer into a function.
jcsp added a commit that referenced this pull request May 24, 2024
…m always yield Err after cancel (#7866)

## Problem

Ongoing hunt for secondary location shutdown hang issues.

## Summary of changes

- Revert the functional changes from #7675 
- Tweak a log in secondary downloads to make it more apparent when we
drop out on cancellation
- Modify DownloadStream's behavior to always return an Err after it has
been cancelled. This _should_ not impact anything, but it makes the
behavior simpler to reason about (e.g. even if the poll function somehow
got called again, it could never end up in an un-cancellable state)

Related #neondatabase/cloud#13576
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a/tech_debt Area: related to tech debt c/storage/pageserver Component: storage: pageserver
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants