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: refine tenant_id->shard lookup #7762

Merged
merged 5 commits into from
May 16, 2024
Merged

Conversation

jcsp
Copy link
Contributor

@jcsp jcsp commented May 14, 2024

Problem

This is tech debt from when shard splitting was implemented, to handle more nicely the edge case of a client reconnect at the moment of the split.

During shard splits, there were edge cases where we could incorrectly return NotFound to a getpage@lsn request, prompting an unwanted reconnect/backoff from the client.

It is already the case that parent shards during splits are marked InProgress before child shards are created, so resolve_attached_shard will not match on them, thereby implicitly preferring child shards (good).

However, we were not doing any elegant handling of InProgress in general: get_active_tenant_with_timeout was previously mostly dead code: it was inspecting the slot found by resolve_attached_shard and maybe waiting for InProgress, but that path is never taken because since ef7c9c2 the resolve function only ever returns attached slots.

Closes: #7044

Summary of changes

  • Change return value of resolve_attached_shard to distinguish between true NotFound case, and the case where we skipped slots that were InProgress.
  • Rework get_active_tenant_with_timeout to loop over calling resolve_attached_shard, waiting if it sees an InProgress result.

The resulting behavior during a shard split is:

  • If we look up a shard early in split when parent is InProgress but children aren't created yet, we'll wait for the parent to be shut down. This corresponds to the part of the split where we wait for LSNs to catch up: so a small delay to the request, but a clean enough handling.
  • If we look up a shard while child shards are already present, we will match on those shards rather than the parent, as intended.

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 14, 2024
@jcsp jcsp requested a review from problame May 14, 2024 21:29
Copy link

github-actions bot commented May 14, 2024

3060 tests run: 2933 passed, 0 failed, 127 skipped (full report)


Flaky tests (3)

Postgres 16

  • test_compute_pageserver_connection_stress: release
  • test_vm_bit_clear_on_heap_lock: release, debug

Code coverage* (full report)

  • functions: 31.5% (6350 of 20174 functions)
  • lines: 47.5% (47944 of 100974 lines)

* collected from Rust tests only


The comment gets automatically updated with the latest test results
146df8f at 2024-05-16T08:39:00.354Z :recycle:

@jcsp jcsp marked this pull request as ready for review May 15, 2024 08:51
@jcsp jcsp requested a review from a team as a code owner May 15, 2024 08:51
pageserver/src/tenant/mgr.rs Outdated Show resolved Hide resolved
pageserver/src/tenant/mgr.rs Outdated Show resolved Hide resolved
pageserver/src/tenant/mgr.rs Outdated Show resolved Hide resolved
@jcsp jcsp enabled auto-merge (squash) May 15, 2024 17:18
@jcsp
Copy link
Contributor Author

jcsp commented May 15, 2024

Interesting failure: while we're waiting for InProgress, we are holding a gate which is blocking location_conf call on a tenant: https://neon-github-public-dev.s3.amazonaws.com/reports/pr-7762/9099973490/index.html#suites/140824de6e814b5b1ae2b622c3f67840/7bae027ac87aa2ef

Need to think about what happens if a page request handler is holding a gate on an ancestor shard during a split.

@jcsp jcsp force-pushed the jcsp/issue-7044-shard-resolve branch from 4ada3de to 146df8f Compare May 16, 2024 07:49
@jcsp jcsp merged commit 03c6039 into main May 16, 2024
50 checks passed
@jcsp jcsp deleted the jcsp/issue-7044-shard-resolve branch May 16, 2024 08:26
a-masterov pushed a commit that referenced this pull request May 20, 2024
## Problem

This is tech debt from when shard splitting was implemented, to handle
more nicely the edge case of a client reconnect at the moment of the
split.

During shard splits, there were edge cases where we could incorrectly
return NotFound to a getpage@lsn request, prompting an unwanted
reconnect/backoff from the client.

It is already the case that parent shards during splits are marked
InProgress before child shards are created, so `resolve_attached_shard`
will not match on them, thereby implicitly preferring child shards
(good).

However, we were not doing any elegant handling of InProgress in
general: `get_active_tenant_with_timeout` was previously mostly dead
code: it was inspecting the slot found by `resolve_attached_shard` and
maybe waiting for InProgress, but that path is never taken because since
ef7c9c2 the resolve function only ever
returns attached slots.

Closes: #7044

## Summary of changes

- Change return value of `resolve_attached_shard` to distinguish between
true NotFound case, and the case where we skipped slots that were
InProgress.
- Rework `get_active_tenant_with_timeout` to loop over calling
resolve_attached_shard, waiting if it sees an InProgress result.

The resulting behavior during a shard split is:
- If we look up a shard early in split when parent is InProgress but
children aren't created yet, we'll wait for the parent to be shut down.
This corresponds to the part of the split where we wait for LSNs to
catch up: so a small delay to the request, but a clean enough handling.
- If we look up a shard while child shards are already present, we will
match on those shards rather than the parent, as intended.
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.

pageserver: skip shutting-down tenant shards in page_service lookup if a split is in progress
2 participants