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

server: cover edge cases for scrubbed checkpoint users #20259

Merged
merged 5 commits into from
Mar 25, 2024

Conversation

znewton
Copy link
Contributor

@znewton znewton commented Mar 21, 2024

Description

Continuing the work from #20150 to make sure we cover all the possible edge cases.

Reviewer Guidance

The control flow in Scribe LambdaFactory was a bit confusing, so hopefully I didn't mess it up.
I understand the current control flow to be:

if (no global checkpoint)
  use Default checkpoint
elsif (global checkpoint was cleared or global checkpoint quorum was scrubbed)
  use Summary checkpoint
else
  use latest DB checkpoint (local or global)

My goal was to make the control flow behave as follows:

if (no global and no local checkpoint and no summary checkpoint)
  use Default checkpoint
elsif (
    global checkpoint was cleared and summary checkpoint ahead of local db checkpoint
    or latest DB checkpoint quorum was scrubbed
    or summary checkpoint ahead of latest DB checkpoint
  )
  use Summary checkpoint
else
  use latest DB checkpoint (local or global)

@github-actions github-actions bot added area: server Server related issues (routerlicious) base: main PRs targeted against main branch labels Mar 21, 2024
latestSummaryCheckpoint = latestSummary.scribe
? JSON.parse(latestSummary.scribe)
: undefined;
latestDbCheckpoint = (await this.checkpointService.restoreFromCheckpoint(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Technically this could be awaited in parallel with the summary read. Code would be messy though.

@znewton znewton marked this pull request as ready for review March 21, 2024 19:54
@znewton znewton merged commit 6718a9a into microsoft:main Mar 25, 2024
30 checks passed
@znewton znewton deleted the server/not-an-azureuser-2 branch March 25, 2024 21:06
tianzhu007 added a commit that referenced this pull request Mar 25, 2024
* main: (36 commits)
  feat(tree): create refreshers during delta visit (#20303)
  Lint against import of @fluidframework/datastore in e2e tests (#20307)
  server: cover edge cases for scrubbed checkpoint users (#20259)
  refactor: Update dev dep on package 'start-server-and-test' (#20298)
  ci: Move templates out of the 1ES folder (#20056)
  Added unit tests to check usage of IRedisClientConnectionManager for Historian and Gitrest (#20306)
  build(test-snapshots): use node16 module resolution (#20233)
  Forbid import of @fluidframework/aqueduct in e2e tests (#20261)
  fix(tree): Make failure to provide id-compressor a usage error (#20282)
  fix(api-markdown-documenter): Reduce package version to correct next version (#20302)
  Added customization for gitrest and historian (#20243)
  fix(build-tools): mixed internal range detection (#18828)
  Removing 'paused session' path from SessionResult Metric (#20294)
  fix(fluid-build): limit Biome config tracking to repo (#20296)
  refactor: Update webpack-dev-server dependency (#20278)
  Create framework for safe rollout of back-compatible runtime document schema changes (#20174)
  Test enabling IdCompressor in RC2 (#20256)
  refactor(tree): Extract leaf schemas into their own module (#20289)
  build(client,build-tools): Upgrade biome to 1.6.2 (#20285)
  feat(build-cli): Add `modify fluid-imports` command (#20006)
  ...
znewton added a commit to znewton/FluidFramework that referenced this pull request May 17, 2024
## Description

Continuing the work from microsoft#20150 to make sure we cover all the possible
edge cases.

## Reviewer Guidance

The control flow in Scribe LambdaFactory was a bit confusing, so
hopefully I didn't mess it up.
I understand the current control flow to be:

```
if (no global checkpoint)
  use Default checkpoint
elsif (global checkpoint was cleared or global checkpoint quorum was scrubbed)
  use Summary checkpoint
else
  use latest DB checkpoint (local or global)
```

My goal was to make the control flow behave as follows:

```
if (no global and no local checkpoint and no summary checkpoint)
  use Default checkpoint
elsif (
    global checkpoint was cleared and summary checkpoint ahead of local db checkpoint
    or latest DB checkpoint quorum was scrubbed
    or summary checkpoint ahead of latest DB checkpoint
  )
  use Summary checkpoint
else
  use latest DB checkpoint (local or global)
```
znewton added a commit that referenced this pull request May 17, 2024
## Description

T9s v4 doesn't have the Quorum scrubbed users fix, and is also missing
the fix for correct Read/Write scopes on socket connection.
Cherry-picking some commits:

---

### server: cover edge cases for scrubbed checkpoint users
(#20259)

6718a9a

---

### server: provided user was not an azure user fixes (#20150)

786abfa

---

### server: fix mutable quorum snapshot (#20329)

5de9472

---

### fix: send correct connection scopes for client (#20312)

7d07fc5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: server Server related issues (routerlicious) base: main PRs targeted against main branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants