Skip to content

Periodic snapshot refresh#21504

Merged
dannimad merged 12 commits into
microsoft:mainfrom
dannimad:periodic-refresh
Jun 27, 2024
Merged

Periodic snapshot refresh#21504
dannimad merged 12 commits into
microsoft:mainfrom
dannimad:periodic-refresh

Conversation

@dannimad
Copy link
Copy Markdown
Contributor

@dannimad dannimad commented Jun 18, 2024

This change triggers fetching the latest snapshot in the serializedStateManager after a timeout established by config provider. Once that is complete, ssm will try to update the base snapshot if the already given conditions are met. Such timer is initialized in three different scenarios:

  1. While setting the initial snapshot while attaching the container.
  2. While loading a container with no pending state.
  3. Once we updated the snapshot for a subsequent refresh.

The scenario in which we load from a container with pending state is already covered by current code.

Here are some scenarios this change is covering, explaining its behavior:

  1. Start fetching once timer is finished, fetching succeeds, conditions to refresh are met, base snapshot is updated, and a timer is restarted for the next attempt. On the next attempt the same procedure is followed.
  2. Start fetching once timer is finished, fetching succeeds, conditions to refresh are not met, wait for all conditions to be met and attempt again via saved event, snapshot is updated.
  3. Start fetching once timer is finished, fetching succeeds, conditions to refresh are met, snapshot is not updated due to being too old to refresh, timer restarts.
  4. Start fetching once timer is finished, fetching does not succeed, attempt again until it succeeds.

Current conditions to update the snapshot should be sufficient since the most complicated case (while loading with pending state) is already covered.

@github-actions github-actions Bot added area: loader Loader related issues area: tests Tests to add, test infrastructure improvements, etc base: main PRs targeted against main branch labels Jun 18, 2024
@msfluid-bot
Copy link
Copy Markdown
Collaborator

msfluid-bot commented Jun 18, 2024

Baseline CI build failed, cannot generate bundle analysis at this time


Baseline commit: d976469

Generated by 🚫 dangerJS against e6a9d5a

Comment thread packages/loader/container-loader/src/serializedStateManager.ts Outdated
@dannimad dannimad marked this pull request as ready for review June 24, 2024 18:03
Comment thread packages/loader/container-loader/src/serializedStateManager.ts Outdated
error,
});
this._refreshSnapshotP = undefined;
this.tryRefreshSnapshot();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

is there anything preventing this from overlapping with the timer? do we do anything make sure only 1 tryRefreshSnapshot is ever happening at once?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

i would probably just skip this, and rely on the driver to retry and suceed, and if it fails it will happen again when the timer triggers again,

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I believe by removing this tryRefreshSnapshot and blocking when latestSnapshot is set should be enough to prevent overlapping but I can wrap this function on a runSingle

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I believe the runSingle approach doesn't work since tryRefreshSnapshot is not async. Conditions should be sufficient to prevent reentrancy.

Comment thread packages/loader/container-loader/src/serializedStateManager.ts Outdated
Comment thread packages/loader/container-loader/src/serializedStateManager.ts
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: loader Loader related issues area: tests Tests to add, test infrastructure improvements, etc base: main PRs targeted against main branch

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants