FluidFramework icon indicating copy to clipboard operation
FluidFramework copied to clipboard

Convert ephemeral container to be specified on attach rather than as driver policy

Open ChumpChief opened this issue 2 years ago • 3 comments

A container is either created as ephemeral/persisted depending on how the createContainer network request is issued. This happens during the attach() flow, and can vary from container to container.

The current implementation in repo sets a flag at driver creation, which is both sooner than needed and more global than desirable (affects all containers created from that driver). By doing this, we can then avoid exposing a new property on AzureClient as in #18928.

This draft PR explores what it might look like to move that to the attach by mimicking the pattern used in ODSP driver to smuggle additional creation params through to the createContainer call (see odspDocumentServiceFactoryCore.ts).

I'm particularly interested in feedback on:

  1. Preferences on the options 1/2 I call out in AzureClient.
  2. Any objections to putting Azure-specific behaviors in routerlicious-driver (versus the overhead of standing up a new package similar to what the ODSP equivalents do). Note that the ephemeral container behavior is already Azure-specific and is already in routerlicious-driver, but this becomes more obvious and apparent with the addition of AzureResolvedUrl.
  3. Any concerns about current customers trying out the ephemeral functionality and their ability to adapt to this (breaking) change. There is no change to the client/server protocol, they would just need to adapt when picking up the new FF version.

ChumpChief avatar Feb 08 '24 00:02 ChumpChief

Could not find a usable baseline build with search starting at CI a2992b5bbb681a1589084a8ac0108f5fe385a308

Generated by :no_entry_sign: dangerJS against 99149aab44ee1f116f0ffc24f7d5fdf60b4c9b81

msfluid-bot avatar Feb 08 '24 01:02 msfluid-bot

One more thing: If the createAsEphemeral flag is added to createFluidContainer, I think we need to update the AzureClient.ts file to pass the flag to all references of createFluidContainer (especially to createContainer, I need this one for the ephemeral E2E tests)

True, looks like I got lazy with the plumbing :-P. I'm a little inclined to go with Option 2 (the more we defer the choice, the more scenarios it can support and the closer it is to the underlying implementation) so wouldn't need the extra plumbing in that case.

ChumpChief avatar Feb 08 '24 18:02 ChumpChief

One more thing: If the createAsEphemeral flag is added to createFluidContainer, I think we need to update the AzureClient.ts file to pass the flag to all references of createFluidContainer (especially to createContainer, I need this one for the ephemeral E2E tests)

True, looks like I got lazy with the plumbing :-P. I'm a little inclined to go with Option 2 (the more we defer the choice, the more scenarios it can support and the closer it is to the underlying implementation) so wouldn't need the extra plumbing in that case.

I see, that's a good point. I agree that option 2 could be a good option for having greater support and less plumbing required

Toritos01 avatar Feb 08 '24 18:02 Toritos01

This PR has been automatically marked as stale because it has had no activity for 60 days. It will be closed if no further activity occurs within 8 days of this comment. Thank you for your contributions to Fluid Framework!