Get rid of createDataStoreWithRealizationFn(). Introduce scope concept at data store creation. #2950
Conversation
vladsud
requested review from
anthony-murphy,
skylerjokiel,
curtisman and
heliocliu
|
I get what you are doing at the runtime layer, but i don't understand the aquaduct layer. I'm confused between props and scope. how those interact with synthesize. it also looks like this a step backwards in terms of typing |
|
Is the expectation that the lower-level API (createDataStore) will not be called directly by consumers and that they will do so through an upper layer API that has any necessary type checking? At least while initial props are the only use case, it seems odd to me that there's some public lower level api that takes anything that allows you to bypass the enforcements from higher level wrappers In reply to: 667328111 [](ancestors = 667328111) |
|
@heliocliu, here is how I look at it: |
…into initialProps # Conflicts: # BREAKING.md # packages/framework/aqueduct/src/componentFactories/sharedComponentFactory.ts # packages/framework/aqueduct/src/components/sharedComponent.ts # packages/runtime/container-runtime/src/containerRuntime.ts # packages/runtime/container-runtime/src/dataStoreContext.ts
| @@ -20,6 +21,7 @@ The following telemetry event names have been updated to drop references to the | |||
| ComponentRuntimeDisposeError -> ChannelDisposeError | |||
| ComponentContextDisposeError -> FluidDataStoreContextDisposeError | |||
| SignalComponentNotFound -> SignalFluidDataStoreNotFound | |||
| >>>>>>> c6c090ca861703c5a6773788d131e201a9424dc2 | |||
Also small name tweaks in pure object initial state logic.
packages/framework/aqueduct/src/componentFactories/sharedComponentFactory.ts
Show resolved
Hide resolved
**Key changes:** 1. IFluidHandleContext no longer implements IFluidRouter. It's request() method is renamed to resolveHandle(). This reduces number of responsibilities for both interfaces and makes it clearer on the purpose of interfaces. Note: This causes DataStoreRuntime to have both request() & resolveHandle(). In next iteration, they will have different behaviors - DDS URIs will fail to resolve when request comes in through request(). I.e. it would be impossible to request DDS from data store unless data store handler implements such access. This is equivalent to making DDSs private on a data store class. 2. 'path' is removed from both IFluidHandle & IFluidRouter. It has been marked deprecated for many versions. 3. Introducing handleFromLegacyUri() for back compat. Using it in couple places in this change to scope amount of churn - all places where it's used needs revisit. More on that below. 4. Fixing how we refer to task manager ID and how we access task manager & agent scheduler. In many places code confuses data store ID vs. type (even though they are the same thing). Exposing task manager on IContainerRuntimeBase. More on future changes here below. 5. Vltava: changing registry creation pattern - hiding component type names, forcing everything to go through factories. Same pattern should be expanded to all of our examples, as name we use in registry not always matches type on a factory. - Also removing using package name as component type - that has pretty big impact on bundle sizes, and is backward compat hazard - type name can't change, which is not obvious looking at package.json. 6. Last edited is simplified - async load path is deprecated, with assumption that it is being used on a critical path of container loading, with detached container creation. 7. PureDataObject method names are busted due to renames for many users (i.e. componentInitializingFirstTime is still used in code in derived classes, when such method was renamed on base class). Follow up for # 3 & 4 above: Next step will be to pass data object dependencies to them directly through scoping mechanism on object creation, and start removing access to "globals" in container. The most obvious example why it's bad is PureDataObject.getService() & getMatchMakerContainerService() implementations. Consumers of those are likely not aware that generateContainerServicesRequestHandler() has to be used by container developer. In fact, getMatchMakerContainerService() is not used in our repo (other than UT)! Better approach is to pass these dependencies through local (to object) scope, and force fluid object implementation to grab and store such dependencies (using handles) within data object itself, for future use. PureDataObjectFactory.instantiateInstance() can validate required dependencies are present. PR #2950 takes a first step on this path.
|
Closing in favor of PR #3112 direction - detached context creation allows factories to own passing context/state directly, without need of runtime to understand such concepts. |
This change completely eliminates createDataStoreWithRealizationFn() and unifies paths how data store objects are created.
The most interesting aspect of change is how we deal with initial props.
In this PR I attempt to keep ability to pass props in Aqueduct (PureDataObject(Factory)), with minimal support form runtime.
And maintain existing single create-init-with-props Aqueduct path (i.e. exposed as a single call to consumers of PureDataObject via initializingFirstTime()). The alternative is to break sequence into “create” and optional “init-with-props” phase and remove the later from Aqueduct, leaving it up to each case to define custom interface for creator and fluid object to talk. I like both approaches, but choosing first one mostly because Fluid Preview heavily depends on a single call here - moving to later suggesting is likely slightly better architecturally, but a bit more work for Fluid Preview app.
To implement existing flow all I need is a Boolean, which tells upfront to PureDataObject if it can expect "init-with-props" call to follow (from PureDataObjectFactory.createInstance()) after instantiation, or not (data store is created not through PureDataObjectFactory path, but directly through calling IContainerRuntimeBase.createDataStore()).
I think that’s good introduction to start moving into notion of scoping environment accessible to fluid objects at creation time and pushing them to “record” environment at creation time and move away from relying on global container state (i.e. IFluidDataStoreContext.scope which comes from ContainerRuntime.containerScope). I.e. create isolation and composability for objects and libraries participating in assembly of container. While we can do so through existing IFluidDataStoreContext.scope, I'm choosing same path as with createProps - slowly eliminate these things from context and rely on direct interaction as specific APIs.
We have started this discussion, but did not finish. Please ping me if you want to discuss it before moving with this direciton.