Expose handle in `FluidDataStoreRuntime`

[alexvy86]

Description

• Add new interface IFluidDataStoreRuntimeEntrypoint which is an IFluidRouter with an optional (for now) handle?: IFluidHandle<FluidObject> property.
• Make ContainerRuntime.getRootDataStore() return the new interface instead of IFluidRouter.
• Make IFluidDataStoreRuntime, IFluidDataStoreChannel, and IDataStore, implement the new interface instead of IFluidRouter.
• Add optional (for now) constructor parameter to FluidDataStoreRuntime for consumers to specify a function that initializes the entry point (FluidObject that the handle ends up pointing to). The handle is only initialized if this function is provided, so all existing code will still behave exactly as it does today, and we can start using this function and the handle in small, manageable steps.
• Update DataStore to set up its new handle property from the one in the underlying IFluidDataStoreChannel.
• Inline some private functions that had a single call-site.

TO-DO

• [ ] Make DataStore context objects access the DataStore runtime's handle to initialize it.
• [ ] Look at usages of getRootDataStore() to see if we can start using handle at call sites controlled by us.
• [ ] Update uses of FluidDataStoreRuntime (including subclasses) to leverage the new constructor parameter.
• [ ] Update implementations of IFluidDataStoreFactory to leverage the new constructor parameter.
• [ ] Update DataObject to use the new handle where relevant.
• [ ] Update end-to-end tests to use the handle instead of the IFluidRouter directly.

Reviewer Guidance

• FluidDataStoreRuntime implements IFluidDataStoreChannel and IFluidDataStoreRuntime, both of which implement IFluidDataStoreChannel (and transitively the new IFluidDataStoreRuntimeEntrypoint interface). The only other implementation of IFluidDataStoreRuntime is MockFluidDataStoreRuntime which also implements IFluidDataStoreChannel. Wondering if there's some overlap here that we can start thinking about consolidating, but not sure.

Other information or known dependencies

AB1700

[msfluid-bot]

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

Generated by :no_entry_sign: dangerJS against 1b3f495975e14a77a272fcffe0b8fc2283abf0ce

[alexvy86]

~~@anthony-murphy in your POC you gave no default to the new constructor parameter for FluidDataStoreRuntime, and that required that a bunch of places be updated immediately with their own functions. That needs to be done that way, right? E.g. AgentSchedulerRuntime doing something different than FluidDataStoreRuntime, so it cannot use a default entrypoint initializer defined in the parent?~~

I realized it needs to be done when the initializeEntrypoint function becomes required but for this PR it's remaining optional, so all existing code can keep working as it works today.

[vladsud]

I'd love to see IFluidDataStoreFactory changes as part of this change, and ideally - at least one example that demonstrates full end-to-end usage. This feels like where I'd start, and usability feels more important than implementation :)

[alexvy86]

I'd love to see IFluidDataStoreFactory changes as part of this change, and ideally - at least one example that demonstrates full end-to-end usage. This feels like where I'd start, and usability feels more important than implementation :)

i also see you have a list of todo in the pr, but i'm unclear if those will happen in this pr, or somewhere else.

My intention was to do them in separate PRs, after "setting the stage up" with this one. Just trying to keep them small and easy to review (and to understand for myself 😄). Start with this, one with actual usage of the new capabilities but keeping them optional, and eventually one to make them non-optional. But it sounds like we prefer to at least start with some of the usage in this PR too?

[alexvy86]

I'd love to see IFluidDataStoreFactory changes as part of this change,

@vladsud are you thinking of literal changes to the interface, or changes to its implementations so they leverage the new functionality when they create data stores? The latter is part of what I'm working on (still locally). So far I didn't intend to change IFluidDataStoreFactory.instantiateDataStore(), just let the implementations leverage the new constructor parameter when they instantiate FluidDataStoreRuntime.

[alexvy86]

I'll add and push some of the local work on expanding the scope of this change to make it more "tangible" without making it breaking yet.

[vladsud]

I'd love to see IFluidDataStoreFactory changes as part of this change,

@vladsud are you thinking of literal changes to the interface, or changes to its implementations so they leverage the new functionality when they create data stores? The latter is part of what I'm working on (still locally). So far I didn't intend to change IFluidDataStoreFactory.instantiateDataStore(), just let the implementations leverage the new constructor parameter when they instantiate FluidDataStoreRuntime.

I want to see actual usage example, i.e. how our customers will consume that feature. It's Ok if it's a draft PR somewhere (maybe does not compile), but I really want to see one of our examples moved to leveraging new functionality.

[anthony-murphy]

My intention was to do them in separate PRs, after "setting the stage up" with this one. Just trying to keep them small and easy to review (and to understand for myself 😄). Start with this, one with actual usage of the new capabilities but keeping them optional, and eventually one to make them non-optional. But it sounds like we prefer to at least start with some of the usage in this PR too?

just to be clear, making them non-optional is much longer term, we should be able to use them while they are still optional

[anthony-murphy]

added a bunch of comments around how we can minimize the surface area impact here, which seems like a good plan to side step the concerns here, and give us time to resolve them in parallel. There is more work than just my comments, but wasn't sure a bunch of redundant comments was necessary, as i'd rather just pair if it is unclear. ping me if you need help

to understand how we do anonymous feature inspection and discovery take a look at: https://github.com/microsoft/FluidFramework/blob/5b7e759b505811cc6a60ac4a0cdf5f8b1c8f2d20/packages/common/core-interfaces/src/provider.ts#L66

and https://github.com/microsoft/FluidFramework/blob/main/packages/common/core-interfaces/src/test/types/fluidObjectTypes.ts

[alexvy86]

Oh, @tyler-cai-microsoft , I had forgotten to tag you in this one. PR is still kind of in flux, but feel free to explore and comment.

[alexvy86]

@vladsud @agarwal-navin if you can do another round of review for this one, it'd be great. We're almost there, I think :).

[agarwal-navin]

@alexvy86 Can you add tests to validate that the data store initialization does not happen unless handle.get() or request() is explicitly called on it. Basically, for summarizer clients, we don't want initialization to happen by default. It looks like it won't happen but the logic is so complex that it can regress.

[anthony-murphy]

@alexvy86 Can you add tests to validate that the data store initialization does not happen unless handle.get() or request() is explicitly called on it. Basically, for summarizer clients, we don't want initialization to happen by default. It looks like it won't happen but the logic is so complex that it can regress.

Let's create a follow up item for this, and not block this PR on it. Its a good gap to fill, but sound like there are tests that will fail (GC), and its not a new scenario created by this PR, just one we should fill.

[agarwal-navin]

Yes, sure. That sounds good.

[alexvy86]

Follow-up item for additional tests here